-
Notifications
You must be signed in to change notification settings - Fork 243
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
Introduce bundle longevity #2586
Conversation
It is used to check keep of the block slot for the last few blocks and used to validate bundle Signed-off-by: linning <[email protected]>
Bundle produced more than the 5 consensus blocks ago will be considered invalid, domain tests are adjusted accordingly Signed-off-by: linning <[email protected]>
79a47fd
to
d3dee67
Compare
Sorry for the force push noise, it is used to fix a clippy warning under |
Does this also allow the ER in the bundle to be not the leaf ER, but an older one? Or was that supported already? |
Summary: This looks like a good idea! I did several quick checks below. Previously, a bundle was supposed to include the "latest" ER. In this new proposal, a bundle just needs to include one of the latest 5 ERs. Let us do some quick sanity checks. Check 1: This will introduce more conflicting domain txs. Does it matter? Check 2: Consensus nodes need to maintain a chain of ERs. How can we ensure this? |
This is not allowed because we are relying on this to ensure the operator is update-to-date and executed the bundles from the latest block, thus the bundle produced by such an operator contains tx that is verified against the latest domain state, if there is an illegal tx in the bundle we can safely slash the operator without worry about if operator just lagging and verified the tx against an older domain state, in this case the bundle will be rejected due to stale ER. The bundle longevity introduced in this PR is about the proof-of-time check, not the ER. |
Let's continue the discussion about ER on the # research channel. In the meantime, the changes in this PR make sense and can be merged and updated on gemini-3h. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. Left some questions
@@ -186,6 +186,7 @@ parameter_types! { | |||
pub const DomainChainByteFee: Balance = 1; | |||
pub const MaxInitialDomainAccounts: u32 = 5; | |||
pub const MinInitialDomainAccountBalance: Balance = SSC; | |||
pub const BundleLongevity: u32 = 5; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cannot we use the BlockSlotCount instead of creating a new type in config ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BlockSlotCount
needs to be greater than BundleLongevity
, and BlockSlotCount
is in pallet-subspace
we need to add a config item in pallet-domains
anyway.
…ckHashCount and BlockSlotCount Signed-off-by: linning <[email protected]>
Signed-off-by: linning <[email protected]>
This PR introduces bundle longevity, for bundle produced from the last 5 consensus blocks won't be considered stale (in main bundle produced before the parent block will be considered as stale), this should ideally fix the issue mentioned at #2516 (comment) and slow domain block production in 3h.
This PR also introduces an additional storage item
BlockSlots
topallet-subspace
to keep track of the slot of the last 6 blocks, it can be used to replace theCurrentSlot
and thecurrent_slot
andparent_slot
fields inParentVoteVerificationData
, but more mitigation code will be required for keeping compatible with 3h, which will be done in a separate PR.Code contributor checklist: