Propose an empty collection if LN can't deduplicate transactions #8320
Conversation
In the unlikely scenario a collection node does not have enough history stored to guarantee it will not include a transaction that has already been included in a past finalized collection, propose an empty collection instead to avoid committing any potential slashing violations. This could potentially happen if a collection node is dynamic-bootstrapped during an epoch it is participating in. See #8222 (comment)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Looks great. Thanks. I updated and extended the documentation a bit (no logical changes) and took the liberty to commit directly to your branch.
I think the logic in this PR is very useful. Though, we might need to be mindful that this PR may not solve the problem entirely (still thinking about it):
Aspect 1
-
Lets assume we have sufficient history of main consensus blocks.
-
We have this index:
main consensus height -> finalized cluster block IDs (set) referencing that height
-
Note that we only populate this index for collections that we know. Just because we have enough main consensus blocks doe snot mean we have enough collections added to this index.
-
Essentially, we are using a heuristic here that collections are synced from the epoch's cluster root block. It would be great if we checked this, maybe in the constructor of the
Builder?
Aspect 2
We should check the same in the mutator:
flow-go/state/cluster/badger/mutator.go
Line 452 in 8c2f842
What do we do if we don't have enough history? A block that is accepted by the compliance engine, is assumed to be valid by HotStuff and probably voted for. If a collector votes for a block that contains duplicates, the collector will be slashed.
So with insufficient history, we can't accept the block in the compliance engine because it might be byzantine. However, we can't reject the block either, because then the node will get stuck if the block is honest.
Suggestion:
- finish this PR with the scope it currently has
- I have created issue #8327, summarizing the edge case.
- I added this to our BFT masterplan doc (notion)
| // Let E be the global transaction expiry constant, measured in blocks. For each | ||
| // T ∈ `includedTransactions`, we have to decide whether the transaction | ||
| // already appeared in _any_ finalized cluster block. | ||
| // Notation: | ||
| // - consider a valid cluster block C and let c be its reference block height | ||
| // - consider a transaction T ∈ `includedTransactions` and let t denote its | ||
| // reference block height | ||
| // First, we determine the height range of main consensus blocks that have to be scanned (for details | ||
| // see method doc). Then Lookup finalized cluster blocks, which reference consensus blocks in that | ||
| // height range. Only those finalized collections can possibly contain transactions still eligible | ||
| // for inclusion in the collection we are building. | ||
| // | ||
| // Boundary conditions: | ||
| // 1. C's reference block height is equal to the lowest reference block height of | ||
| // all its constituent transactions. Hence, for collection C to potentially contain T, it must satisfy c <= t. | ||
| // 2. For T to be eligible for inclusion in collection C, _none_ of the transactions within C are allowed | ||
| // to be expired w.r.t. C's reference block. Hence, for collection C to potentially contain T, it must satisfy t < c + E. | ||
| // | ||
| // Therefore, for collection C to potentially contain transaction T, it must satisfy t - E < c <= t. | ||
| // In other words, we only need to inspect collections with reference block height c ∈ (t-E, t]. | ||
| // Consequently, for a set of transactions, with `minRefHeight` (`maxRefHeight`) being the smallest (largest) | ||
| // reference block height, we only need to inspect collections with c ∈ (minRefHeight-E, maxRefHeight]. | ||
|
|
||
| // the finalized cluster blocks which could possibly contain any conflicting transactions |
There was a problem hiding this comment.
I have consolidated the detailed technical documentation on how we compute the main chain height interval into the method findRefHeightSearchRangeForConflictingClusterBlocks. Added more high-level doc here.
…-propose-empty-collection-if-cant-deduplicate-txs
Co-authored-by: Alexander Hentschel <[email protected]>
In the unlikely scenario a collection node does not have enough history stored to guarantee it will not include a transaction that has already been included in a past finalized collection, propose an empty collection instead to avoid committing any potential slashing violations.
This could potentially happen if a collection node is dynamic-bootstrapped during an epoch it is participating in.
See #8222 (comment)