Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nodes will crash if both nDelayTranches and zerothDelayTrancheWidth are set to zero. #3886

Closed
2 tasks done
david9991 opened this issue Mar 29, 2024 · 3 comments · Fixed by #3897
Closed
2 tasks done
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@david9991
Copy link

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

Nodes will crash if both nDelayTranches and zerothDelayTrancheWidth are set to zero.

Version: 1.9.0-3c3d6fceb82

   0: sp_panic_handler::set::{{closure}}
   1: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/boxed.rs:2021:9
      std::panicking::rust_panic_with_hook
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:783:13
   2: std::panicking::begin_panic_handler::{{closure}}
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:649:13
   3: std::sys_common::backtrace::__rust_end_short_backtrace
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/sys_common/backtrace.rs:170:18
   4: rust_begin_unwind
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:645:5
   5: core::panicking::panic_fmt
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:72:14
   6: core::panicking::panic
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:127:5
   7: <polkadot_node_core_approval_voting::criteria::RealAssignmentCriteria as polkadot_node_core_approval_voting::criteria::AssignmentCriteria>::compute_assignments
   8: polkadot_node_core_approval_voting::import::handle_new_head::{{closure}}
   9: polkadot_node_core_approval_voting::handle_from_overseer::{{closure}}
  10: polkadot_node_core_approval_voting::run::{{closure}}
  11: <futures_util::future::try_future::MapErr<Fut,F> as core::future::future::Future>::poll
  12: polkadot_overseer::spawn::{{closure}}
  13: <tracing_futures::Instrumented<T> as core::future::future::Future>::poll
  14: tokio::runtime::task::raw::poll
  15: std::sys_common::backtrace::__rust_begin_short_backtrace
  16: core::ops::function::FnOnce::call_once{{vtable.shim}}
  17: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/boxed.rs:2007:9
      <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/boxed.rs:2007:9
      std::sys::unix::thread::Thread::new::thread_start
             at rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/sys/unix/thread.rs:108:17
  18: <unknown>
  19: <unknown>


Thread 'tokio-runtime-worker' panicked at 'attempt to calculate the remainder with a divisor of zero', polkadot/node/core/approval-voting/src/criteria.rs:214

This is a bug. Please report it at:

	https://github.com/paritytech/polkadot-sdk/issues/new

Steps to reproduce

Set both nDelayTranches and zerothDelayTrancheWidth to zero via sudo.configuration.

@david9991 david9991 added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Mar 29, 2024
@burdges
Copy link

burdges commented Mar 29, 2024

Is there a testnet trying to do this?

zerothDelayTrancheWidth could maybe be removed. It exists so multiple delay tranches can be collected into tranche zero like we do for modulo tranches. You'd do this if you thought modulo tranches did not make sense, like because their randomness came from something like the candidate hash. I'd originally envisioned doing that when we've realy chain equivocations, but we'll could instead use each relay chain block hash seperately instead, for which modulo tranches again make sense. The advantage of doing candidate hashes was that you can deduplicate the work across the equivocations, but if we charge high enough fees for equivocations then this no longer matters. The advantage of the equivocations looking exactly like the regular system is that hte same security analysis can be used.

@sandreim
Copy link
Contributor

sandreim commented Mar 29, 2024

@david9991 what exactly are you trying to achieve using these parameters? These values don't actually make any sense, but I will craft a fix to prevent the division by zero.

@david9991
Copy link
Author

I did it by mistake, and bricked my testnet.

github-merge-queue bot pushed a commit that referenced this issue Apr 1, 2024
fixes #3886

---------

Signed-off-by: Andrei Sandu <[email protected]>
pgherveou pushed a commit that referenced this issue Apr 2, 2024
fixes #3886

---------

Signed-off-by: Andrei Sandu <[email protected]>
Ank4n pushed a commit that referenced this issue Apr 9, 2024
fixes #3886

---------

Signed-off-by: Andrei Sandu <[email protected]>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this issue Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants