feat: add support for u64 MARF blob offsets#6949
feat: add support for u64 MARF blob offsets#6949francesco-stacks wants to merge 24 commits intostacks-network:developfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6949 +/- ##
===========================================
+ Coverage 54.68% 60.58% +5.90%
===========================================
Files 412 412
Lines 218662 219172 +510
Branches 338 338
===========================================
+ Hits 119571 132790 +13219
+ Misses 99091 86382 -12709
... and 266 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
francesco-stacks
requested review from
aaronb-stacks,
federico-stacks and
jcnelson
francesco-stacks
force-pushed
the
feat/marf-u64-offset-pointers
branch
from
af686ee to
b97d711
Compare
francesco-stacks
force-pushed
the
feat/marf-u64-offset-pointers
branch
from
b97d711 to
9bb657e
Compare
francesco-stacks
changed the title
|
Thanks for putting this together. Regarding the alternatives, I don’t have additional options to propose beyond the ones already listed, as we discussed these approaches over the past few days. I do have a thought instead about the current solution u64-based. Do we expect the long-term trajectory to have only snapshotted nodes? If that’s the case, and considering that snapshotted nodes should require significantly less disk space, it might be reasonable to “spend” a bit more space and move toward a single u64 representation. During the transition we could still support the current u32/u64 duality, but eventually simplify the model by standardizing on u64. |
There was a problem hiding this comment.
Have you considered using variable-length encoding instead? i.e. for on-disk representation, but still using u64 on the type itself?
The tradeoff is marginally more CPU work at the serialization boundary, but you'd pack 5GB into 5 bytes instead of 8, and lower offsets would likely shave some bytes:
- 1 byte: up to 2^7 - 1 = 127
- 2 bytes: up to 2^14 - 1 = 16,383
- 3 bytes: up to 2^21 - 1 = 2,097,151
- 4 bytes: up to 2^28 - 1 = 268,435,455
- 5 bytes: up to 2^35 - 1 = 34,359,738,367
I have a u64-based LEB128 implementation which we use in sBTC here which was optimized for explicit safety guarantees, but a more performance/alloc-optimized impl would be equally simple.
|
Summarizing notes from a call @francesco-stacks and I had:
|
| if is_u64_ptr(encoded_id) { | ||
| w.write_all(&self.ptr().to_be_bytes())?; | ||
| } else { | ||
| w.write_all(&(self.ptr() as u32).to_be_bytes())?; |
There was a problem hiding this comment.
Instead of doing a cast to u32, could you do u32::try_from(self.ptr()).expect("unreachable") so we deliberately crash if the u64 control bit is unset but the ptr can't fit in a u32?
There was a problem hiding this comment.
Even better imo would be to add an Error::IntegerConversion variant and return an Err instead of expecting since the method is fallible anyway? Here and elsewhere.
There was a problem hiding this comment.
Both great points, I moved to try_from and from wherever possible, and use the OverFlowError that was already available
| if is_u64_ptr(encoded_id) { | ||
| w.write_all(&self.ptr().to_be_bytes())?; | ||
| } else { | ||
| w.write_all(&(self.ptr() as u32).to_be_bytes())?; |
There was a problem hiding this comment.
Same here -- let's deliberately panic instead of casting and potentially silently corrupting.
There was a problem hiding this comment.
I return an error now
| let back_block = u32::from_be_bytes([bytes[10], bytes[11], bytes[12], bytes[13]]); | ||
| (ptr, back_block) | ||
| } else { | ||
| let ptr = u32::from_be_bytes([bytes[2], bytes[3], bytes[4], bytes[5]]) as u64; |
There was a problem hiding this comment.
In general, we try to avoid casting. Can you use u64::from() here?
There was a problem hiding this comment.
done. (Sorry, I distributed the porting to from and try_from in different commits)
Pull Request Test Coverage Report for Build 23788516993Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR extends MARF trie storage to support >4GB trie blob offsets by adding a mixed-width u32/u64 pointer encoding (using a 0x20 control bit) while preserving existing 32-bit formats, and adds schema “read-only compatibility check” behavior.
Changes:
- Add mixed u32/u64
TriePtrencoding/decoding (incl. compressed forms) and update byte-length calculations accordingly. - Update trie dump/load and file-backed reads to use
u64offsets, including a multi-pass layout stabilization for dumps. - Extend MARF DB migration to support a “readonly compatibility check only” mode, and add tests for new pointer/patch behaviors.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
stackslib/src/chainstate/stacks/index/node.rs |
Introduces u64-pointer control bit (0x20), updates TriePtr to u64, and adjusts encoding/size logic. |
stackslib/src/chainstate/stacks/index/bits.rs |
Updates pointer list sizing and parsing to handle mixed-width pointers; adjusts seeks accordingly. |
stackslib/src/chainstate/stacks/index/storage.rs |
Propagates u64 offsets through in-memory/dump logic; adds multi-pass dump layout stabilization; wires readonly migration behavior. |
stackslib/src/chainstate/stacks/index/file.rs |
Removes u32 casts when seeking to trie offsets (now u64). |
stackslib/src/chainstate/stacks/index/trie_sql.rs |
Adds readonly mode to migration function for compatibility checking without mutating schema. |
stackslib/src/chainstate/stacks/index/test/* |
Updates tests for u64 pointers; adds patch serialization tests and dump-compression behavior tests (some ignored due to size). |
changelog.d/marf-u64-offset-pointers.added |
Adds changelog fragment for u64 pointer offset support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Normal nodes with forward ptrs need re-measurement; patch | ||
| // nodes have a fixed size independent of pointer encoding. | ||
| let fwd = has_fwd && patch_node_opt.is_none(); | ||
| if let Some((hash_bytes, patch)) = patch_node_opt.take() { | ||
| node_data.push(DumpPtr::Patch(pointer, hash_bytes, patch)); | ||
| } else { | ||
| node_data.push(DumpPtr::Normal(pointer)); | ||
| } | ||
| offsets.push(ptr as u32); | ||
| has_forward_ptrs.push(fwd); | ||
| } | ||
|
|
||
| assert_eq!(offsets.len(), node_data.len()); | ||
| // step 2: repeatedly lay out nodes until serialized offsets stabilize | ||
| // The first 32 bytes are reserved for the parent block hash, | ||
| // and the next 4 bytes for the local block identifier. | ||
| let mut end_offset = BLOCK_HEADER_HASH_ENCODED_SIZE as u64 + 4; | ||
| let mut offsets = vec![]; | ||
| // Cached byte lengths: leaf / pointer-free / patch node sizes are | ||
| // constant across passes, so we only recompute non-leaf nodes. | ||
| let mut byte_lens = node_data | ||
| .iter() | ||
| .map(|dp| { | ||
| let byte_len = if let Some(patch) = dp.patch() { | ||
| TRIEHASH_ENCODED_SIZE + patch.size() | ||
| } else { | ||
| let (node, _) = self.get_nodetype(dp.ptr())?; | ||
| get_node_byte_len_compressed(node) | ||
| }; |
There was a problem hiding this comment.
In dump_compressed_consume, patch node sizes are treated as fixed across layout passes (fwd = has_fwd && patch_node_opt.is_none()), but TrieNodePatch::size() depends on TriePtr::compressed_size(), which can widen from u32->u64 once the child offsets exceed u32::MAX. This can cause incorrect offsets and corrupted serialized output for large tries. Consider marking patch nodes as needing re-measurement as well and recomputing TRIEHASH_ENCODED_SIZE + patch.size() each pass (or recomputing byte lengths for all nodes each pass) so the convergence loop accounts for pointer-width changes inside patches.
| let node_forward = node | ||
| .ptrs() | ||
| .iter() | ||
| .filter(|p| !p.is_empty() && !is_backptr(p.id)) | ||
| .map(|p| p.chr()); | ||
| let diff_forward = patch_node | ||
| .ptr_diff | ||
| .iter() | ||
| .filter(|p| !p.is_empty() && !is_backptr(p.id)) | ||
| .map(|p| p.chr()); | ||
| assert!(node_forward.eq(diff_forward)); |
There was a problem hiding this comment.
The assert!(node_forward.eq(diff_forward)); in dump_compressed_consume will panic the process on mismatch. Since this code runs in non-test builds, it would be safer to convert this into a handled error (e.g., Err(Error::CorruptionError(...))) or at least debug_assert! so a malformed patch/node mismatch can't crash a node in production.
| let node_forward = node | |
| .ptrs() | |
| .iter() | |
| .filter(|p| !p.is_empty() && !is_backptr(p.id)) | |
| .map(|p| p.chr()); | |
| let diff_forward = patch_node | |
| .ptr_diff | |
| .iter() | |
| .filter(|p| !p.is_empty() && !is_backptr(p.id)) | |
| .map(|p| p.chr()); | |
| assert!(node_forward.eq(diff_forward)); | |
| let node_forward: Vec<_> = node | |
| .ptrs() | |
| .iter() | |
| .filter(|p| !p.is_empty() && !is_backptr(p.id)) | |
| .map(|p| p.chr()) | |
| .collect(); | |
| let diff_forward: Vec<_> = patch_node | |
| .ptr_diff | |
| .iter() | |
| .filter(|p| !p.is_empty() && !is_backptr(p.id)) | |
| .map(|p| p.chr()) | |
| .collect(); | |
| if node_forward != diff_forward { | |
| return Err(Error::CorruptionError( | |
| "Forward child pointers of node and patch diff do not match".into(), | |
| )); | |
| } |
| /// In-RAM trie storage. | ||
| /// Used by TrieFileStorage to buffer the next trie being built. | ||
| /// | ||
| /// Pointer in `TrieRAM` are index-based, not disk-offset-based: |
There was a problem hiding this comment.
Doc comment grammar: "Pointer in TrieRAM are" should be "Pointers in TrieRAM are".
| /// Pointer in `TrieRAM` are index-based, not disk-offset-based: | |
| /// Pointers in `TrieRAM` are index-based, not disk-offset-based: |
francesco-stacks
requested review from
cylewitruk-stacks,
federico-stacks and
jcnelson
federico-stacks
left a comment
federico-stacks
left a comment
There was a problem hiding this comment.
implementation looks fine to me. Just added a minor consideration about TrieRAM api.
| /// Pointers in `TrieRAM` are index-based, not disk-offset-based: | ||
| /// `TriePtr::ptr()` is treated as an in-memory node index into `data`, and | ||
| /// traversal/indexing paths are intentionally bounded to `u32`. | ||
| /// Large `u64` byte offsets are only materialized when serializing this trie | ||
| /// to persistent storage (see `dump_consume`/`write_trie_indirect`). | ||
| #[derive(Clone)] | ||
| pub struct TrieRAM<T: MarfTrieId> { |
There was a problem hiding this comment.
I noticed a small API asymmetry in TrieRAM: some methods accept u64 indices (write_nodetype, last_ptr, read_nodetype), while others still take u32 (write_node_hash, get_nodetype).
The documentation mentions that TrieRAM indices are intentionally bounded to u32, but this invariant isn’t currently reflected in the type signatures. There’s no practical impact, since in-memory node counts won’t approach u32::MAX, but it does introduce a bit of inconsistency in the API.
Would it make sense to standardize on one type, either in this PR or a follow-up (or not at all)?
Description
Summary
This PR adds support for 64-bit trie pointers while keeping the current 32-bit format.
I hit this while building a mainnet snapshot: the squashed MARF trie for clarity was around 5GB, so some offsets went past
u32::MAXand crashed. This PR is a prerequisite for squash work, split out so we can align on the approach first.I looked at three options:
Add 64-bit pointers (this PR)
TriePtr, but not a problem for "squashed MARFs")u32, while squashed nodes useu64Split snapshot into multiple smaller tries
Store snapshot in a plain key/value DB
Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/property-testing.md)rpc/openapi.yamlfor RPC endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo