[CHIA-3854] Add INTERNED_GENERATOR cost model to BlockBuilder#1413
[CHIA-3854] Add INTERNED_GENERATOR cost model to BlockBuilder#1413richardkiss wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates BlockBuilder cost accounting to support the INTERNED_GENERATOR hard-fork cost model, and bumps the wheel build tooling to a newer maturin version.
Changes:
- Add
ConsensusFlagstoBlockBuilderand switch generator cost calculation to interned-tree cost whenINTERNED_GENERATORis set. - Recompute exact generator cost during
finalize()by re-parsing + interning serialized bytes (validator-aligned). - Bump maturin requirement for wheel builds to
~=1.8.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
wheel/pyproject.toml |
Bumps maturin version requirement for building the Python wheel. |
crates/chia-consensus/src/build_compressed_block.rs |
Adds flags-aware BlockBuilder and implements INTERNED_GENERATOR cost accounting + updated tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Compute the generator (byte) cost. When INTERNED_GENERATOR is active, | ||
| /// interns the live allocator tree rooted at the current spend list and | ||
| /// returns the deduplicated storage cost. Otherwise falls back to the | ||
| /// serialized byte count plus the two closing bytes. | ||
| fn compute_byte_cost(&self, constants: &ConsensusConstants) -> Result<u64> { | ||
| if self.flags.contains(ConsensusFlags::INTERNED_GENERATOR) { | ||
| // Build a temporary (q . current_spend_list) root for interning. | ||
| // We can't mutate self.allocator here, so we clone enough context | ||
| // by using intern_tree_limited on the existing allocator. The | ||
| // current_spend_list still ends with sentinel (a pair), which | ||
| // contributes a tiny negligible amount to cost — acceptable for | ||
| // an in-progress estimate. | ||
| let interned = | ||
| intern_tree_limited(&self.allocator, self.current_spend_list, u32::MAX as usize) | ||
| .map_err(|_| { | ||
| crate::error::Error::Custom("intern_tree_limited failed".to_string()) | ||
| })?; | ||
| Ok(total_cost_from_tree(&interned)) | ||
| } else { | ||
| Ok((self.ser.size() + 2) * constants.cost_per_byte) | ||
| } |
There was a problem hiding this comment.
In the INTERNED_GENERATOR path, compute_byte_cost() interns self.current_spend_list, which is the inner spend list ending in sentinel and does not include the outer list + (q . ...) wrapper that run_block_generator2() interns from serialized bytes. This systematically underestimates generator cost vs the validator and can allow add_spend_bundles() to accept a block that later exceeds max_block_cost_clvm (potentially tripping the assert! in finalize()). Consider computing the estimate by interning the current serialized bytes (self.ser.get_ref()) like finalize() does, or otherwise ensure the estimate includes the wrapper + nil-termination and is never lower than the final cost.
| .map_err(|_| { | ||
| crate::error::Error::Custom("intern_tree_limited failed".to_string()) |
There was a problem hiding this comment.
intern_tree_limited(...).map_err(|_| Error::Custom("intern_tree_limited failed"...)) discards the underlying error, which makes debugging failures (e.g. depth/size limits) much harder. Preserve the original error message/context in the custom error (or plumb through a dedicated error variant) so callers can see why interning failed.
| .map_err(|_| { | |
| crate::error::Error::Custom("intern_tree_limited failed".to_string()) | |
| .map_err(|e| { | |
| crate::error::Error::Custom(format!("intern_tree_limited failed: {e}")) |
|
|
||
| impl BlockBuilder { | ||
| pub fn new() -> Result<Self> { | ||
| pub fn new(flags: ConsensusFlags) -> Result<Self> { |
There was a problem hiding this comment.
BlockBuilder::new() now requires flags, but there are existing Rust call sites still using the old no-arg constructor (e.g. fuzz targets under crates/chia-consensus/fuzz/fuzz_targets/). Update those call sites (likely ConsensusFlags::empty() or derived flags) or add a compatibility constructor to avoid breaking builds that include fuzz targets.
| pub fn new(flags: ConsensusFlags) -> Result<Self> { | |
| /// Compatibility constructor that uses empty consensus flags. | |
| /// This preserves the older `BlockBuilder::new()` API for callers | |
| /// (e.g. fuzz targets) that don't specify flags explicitly. | |
| pub fn new() -> Result<Self> { | |
| Self::new_with_flags(ConsensusFlags::empty()) | |
| } | |
| /// Constructor that allows specifying consensus flags explicitly. | |
| pub fn new_with_flags(flags: ConsensusFlags) -> Result<Self> { |
| #[new] | ||
| pub fn py_new() -> PyResult<Self> { | ||
| Ok(Self::new()?) | ||
| pub fn py_new(flags: u32) -> PyResult<Self> { |
There was a problem hiding this comment.
The Python #[new] constructor now requires a flags argument. There are existing Python call sites in this repo still constructing BlockBuilder() with no args (e.g. tests/test_block_builder.py), which will break at runtime. Either update those call sites/tests to pass flags derived from get_flags_for_height_and_constants(), or provide a default value for flags to preserve backwards compatibility if the break is not intended.
| pub fn py_new(flags: u32) -> PyResult<Self> { | |
| #[pyo3(signature = (flags=None))] | |
| pub fn py_new(flags: Option<u32>) -> PyResult<Self> { | |
| let flags = flags.unwrap_or(0); |
| let serialized = self.ser.get_ref().clone(); | ||
| let mut decode_alloc = Allocator::new(); | ||
| let root = node_from_bytes_backrefs(&mut decode_alloc, &serialized)?; |
There was a problem hiding this comment.
In finalize() INTERNED_GENERATOR path, let serialized = self.ser.get_ref().clone(); makes a full copy of the generator bytes. You can avoid this extra allocation by taking ownership of the buffer once (e.g. via into_inner()), computing cost from that slice, and then returning the same Vec<u8> to the caller.
| let serialized = self.ser.get_ref().clone(); | |
| let mut decode_alloc = Allocator::new(); | |
| let root = node_from_bytes_backrefs(&mut decode_alloc, &serialized)?; | |
| let serialized = self.ser.get_ref(); | |
| let mut decode_alloc = Allocator::new(); | |
| let root = node_from_bytes_backrefs(&mut decode_alloc, serialized)?; |
| intern_tree_limited(&decode_alloc, root, u32::MAX as usize).map_err(|_| { | ||
| crate::error::Error::Custom("intern_tree_limited failed".to_string()) |
There was a problem hiding this comment.
Same as above: the map_err(|_| Error::Custom("intern_tree_limited failed"...)) here drops the underlying interning error. Including the original error details would make failures diagnosable.
| intern_tree_limited(&decode_alloc, root, u32::MAX as usize).map_err(|_| { | |
| crate::error::Error::Custom("intern_tree_limited failed".to_string()) | |
| intern_tree_limited(&decode_alloc, root, u32::MAX as usize).map_err(|e| { | |
| crate::error::Error::Custom(format!("intern_tree_limited failed: {e}")) |
| flags: ConsensusFlags, | ||
|
|
||
| // the cost of the block we've built up so far, not including the byte-cost. | ||
| // That's seprated out into the byte_cost member. |
There was a problem hiding this comment.
Typo in the comment: "That's seprated out" → "That's separated out".
| // That's seprated out into the byte_cost member. | |
| // That's separated out into the byte_cost member. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is kicking off a free cloud agent to fix this issue. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.
| .map_err(|_| { | ||
| crate::error::Error::Custom("intern_tree_limited failed".to_string()) | ||
| })?; | ||
| Ok(total_cost_from_tree(&interned)) |
There was a problem hiding this comment.
Interned cost estimate only reflects latest batch, not cumulative
High Severity
When INTERNED_GENERATOR is active, compute_byte_cost interns self.current_spend_list, but each add_spend_bundles call rebuilds the spend list starting from self.sentinel (line 169), not from the previous current_spend_list. In the allocator, the latest batch's list (e.g. (item_b . sentinel)) is disconnected from prior batches' nodes (e.g. (item_a . sentinel)). So intern_tree_limited only traverses the latest batch's subtree, missing all previously added items. This causes byte_cost to be severely underestimated, allowing blocks to be overfilled. The non-interned path works correctly because self.ser.size() tracks cumulative size. In finalize, the exact cost from the full serialized tree can then exceed max_block_cost_clvm, triggering the assertion panic.
Additional Locations (1)
|
thinking about this; I think it might be simpler to add a separate builder class for building blocks with the new cost model. I believe almost all of the complexity of the current With the new cost model, the serialization isn't an important part anymore, we can just use the original function to serialize a complete generator at the end (which is simpler). I think the majority of complexity with creating blocks with the new model is unrelated to the existing builder. I think you, essentially, need an incremental version of the interning function. You need to add bundles one at a time until you exceed the cost, and then undo the last one. |
richardkiss
force-pushed
the
block-builder-interned-cost
branch
from
15804bb to
67390ac
Compare
|
Rewrote per Arvid's feedback — now a separate Key differences:
Also bumps maturin |
richardkiss
force-pushed
the
pure-storage-model-v2
branch
from
c6c9941 to
f2dbd51
Compare
richardkiss
force-pushed
the
block-builder-interned-cost
branch
from
67390ac to
150491e
Compare
Coverage Report for CI Build 24760798673Coverage decreased (-0.5%) to 80.172%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
danieljperry
changed the title
A clean, separate builder that avoids the Serializer/sentinel/restore complexity of BlockBuilder. Cost is computed from total_cost_from_tree on the interned quoted generator tree after each add; serialization happens once in finalize() via node_to_bytes_backrefs. Includes Python bindings matching the BlockBuilder interface. Made-with: Cursor
richardkiss
force-pushed
the
block-builder-interned-cost
branch
from
150491e to
84cf5f5
Compare
|
Absorbed into #1418 (unified HF2 scratch PR). InternedBlockBuilder commit cherry-picked there. |
Summary
ConsensusFlagsand uses the interned tree cost model whenINTERNED_GENERATORis set, matching whatrun_block_generator2computes during validation~=1.8for self-contained Rust bootstrapping —pip installfrom a git branch now auto-installs the Rust toolchain if missing (no manual setup needed in CI)BlockBuilder changes
When
INTERNED_GENERATORis active:add_spend_bundles()computes incremental cost viaintern_tree_limited+total_cost_from_treeon the live allocator tree, instead ofser.size() * cost_per_bytefinalize()re-parses the serialized bytes and interns for exact cost, matching the validatorPython API change
BlockBuilder()→BlockBuilder(flags)whereflagscomes fromget_flags_for_height_and_constants(). This keeps the height-to-flags derivation in one place.Test
test_build_blockis parameterized for both flag states, verifyingassert_eq!(conds.cost, cost)againstrun_block_generator2with matching flags.Context
Addresses Arvid's review feedback on PR #1377: after the INTERNED_GENERATOR hard fork, chia-blockchain needs to be able to build blocks with correct cost estimates. This PR updates BlockBuilder so
create_block_generator2(the production farming path) will produce correct costs.Depends on #1377.
Test plan
cargo check -p chia-consensuspassescargo test -p chia-consensus -- build_compressed_blockpasses (ignored tests needtest-bundles/data)Made with Cursor
Note
Medium Risk
Changes block cost accounting to match the post-fork
INTERNED_GENERATORvalidation model and updates the Python constructor signature, which could affect block packing behavior and downstream callers if mis-estimated.Overview
Adds flag-driven generator byte-cost accounting to
BlockBuilderso it can estimate and finalize block cost using theINTERNED_GENERATORinterned-tree model instead of serialized-size-per-byte when the flag is set.BlockBuilder::newnow takesConsensusFlags(and PythonBlockBuilder(flags)follows suit), tracks the current spend-list head for incremental cost estimation, andfinalize()re-parses serialized bytes to compute an exact interned-tree cost matching the validator.Tests are refactored to run the expensive block-building round-trip under both flag modes, and the Python wheel build bumps
maturinto~=1.8.Written by Cursor Bugbot for commit 15804bb. This will update automatically on new commits. Configure here.