Repository navigation
test(chain): add missing prev_blockhash cases to merge_chains_with_prev_blockhash - #2412
ravindra-RKB wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2412 +/- ##
=======================================
Coverage 80.66% 80.66%
=======================================
Files 31 31
Lines 6236 6236
Branches 293 293
=======================================
Hits 5030 5030
Misses 1136 1136
Partials 70 70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
50f0627 to
e0b475f
Compare
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
Only one case covers something new. The other five either repeat existing cases with one more block, or the update holds every height involved, so prev_blockhash is never consulted and a BlockHash chain gives the same result (all the evictions come from the ordinary invalidation rule). The PR body says they test prev_blockhash behaviour, but they don't. Per-case comments below.
Other:
| // The update contains contiguous blocks B (height 2) and C (height 3) without genesis. | ||
| // B's `prev_blockhash` connects to chain's A at height 1. | ||
| TestLocalChain { | ||
| name: "prev_blockhash extends chain with multiple blocks without explicit genesis", |
There was a problem hiding this comment.
design: This is the previous case with one more block. C.prev_blockhash = B points inside the update, so the only placeholder that matters is still A from B, which the case above already covers. Drop it?
| //! | ||
| //! - `std` (default): use the standard library, including its `HashMap`/`HashSet`. Without it, | ||
| //! this crate is `no_std` and uses `alloc`'s `BTreeMap`/`BTreeSet` instead. | ||
| //! - `std` (default): use the standard library, including its `HashMap`/`HashSet`. Without it, this |
| // Chain has a gap at height 1. The update provides block A (filling the gap and satisfying | ||
| // B's `prev_blockhash`) and extends the tip to height 3 with C. | ||
| TestLocalChain { | ||
| name: "update fills gap and extends tip with matching prev_blockhashes", |
There was a problem hiding this comment.
design: This is update fills gap with matching prev_blockhash (:1279) plus C on top. Extending the tip isn't a prev_blockhash path, and the update holds every height, so a BlockHash chain gives the same changeset. Drop it?
| // Update reorganizes heights 2 and 3 simultaneously. Both B' and D' have updated | ||
| // `prev_blockhash` references that chain together (`D' -> B' -> A`). | ||
| TestLocalChain { | ||
| name: "multi-block reorg with matching heights and updated prev_blockhashes", |
There was a problem hiding this comment.
design: The update has every height from 0 to 3, so no placeholder is created and prev_blockhash is never checked against the original chain. It's a plain reorg, and the BlockHash cases already cover that (transitive invalidation applies to checkpoints higher than invalidation). Drop it?
| // Update forks at height 1 and extends to height 2 on the alternate chain. Height 3 of | ||
| // the original chain is evicted, and heights 1 and 2 are updated. | ||
| TestLocalChain { | ||
| name: "reorg at height 1 replaces chain with alternate fork", |
There was a problem hiding this comment.
design: Height 3 isn't evicted through prev_blockhash: the update conflicts at height 1, below its tip, so merge_chains drops every original block above that anyway (same as the BlockHash transitive invalidation cases). cascading eviction through multiple blocks (:1304) already covers eviction via prev_blockhash. The PR body's "evicts higher chain blocks that depend on the original chain via prev_blockhash" doesn't describe what this tests. Drop it?
| // (at height 1) and the update (at height 2) have blocks above genesis that do not | ||
| // overlap or resolve, connection fails at height 1. | ||
| TestLocalChain { | ||
| name: "prev_blockhash cannot connect when update references unknown block across gap", |
There was a problem hiding this comment.
design: This fails for the BlockHash reason, not because of X: the chain's A sits between the point of agreement (genesis) and the update's next block, and the update doesn't include height 1. With BlockHash data (_, then C at 3) you get the same try_include_height: 1, the rule update and chain does not connect (:212) tests. It's also unverifiable connection despite shared genesis (:1223) with chain and update swapped. Drop it?
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
Additionally, commits need to be signed otherwise merging won't be possible.
…v_blockhash Extend merge_chains_with_prev_blockhash in est_local_chain.rs to verify that an update connecting via an inferred prev_blockhash placeholder merges correctly without requiring an explicit genesis checkpoint in the update.
e0b475f to
086a748
Compare
|
Thanks for the detailed review @evanlinjin! I have addressed all the feedback:
|
|
Thanks @ravindra-RKB, and I'm sorry: I got this one wrong. In my review I asked you to keep the "update connection without explicit genesis" case because I thought nothing covered it. On a second look, Apologies for the extra round of trimming, rebasing and signing I asked of you. You handled the feedback quickly and cleanly. If you'd like to keep working on |
|
No worries at all @evanlinjin, appreciate the clarification and the thorough review! I'll take a look at the |
Description
Following up on the suggestion in #2410 regarding test coverage for
merge_chainscorrectness, this PR adds a test case tomerge_chains_with_prev_blockhashincrates/chain/tests/test_local_chain.rs:Verifies that an update starting at height > 1 with a valid
prev_blockhashconnects via an inferred placeholder checkpoint without requiring an explicit genesis checkpoint in the update.Notes to the reviewers
The test case is written using the existing
TestLocalChaintable-driven runner incrates/chain/tests/test_local_chain.rsand passes against the current implementation.Changelog notice
none
Checklists
All Submissions: