[LIMIT_HEAP] Return error if LIMIT_HEAP passed to run_block_generator2 - #1396
Draft
richardkiss wants to merge 13 commits into
Draft
[LIMIT_HEAP] Return error if LIMIT_HEAP passed to run_block_generator2#1396richardkiss wants to merge 13 commits into
richardkiss wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the consensus/runtime boundary explicit by rejecting the mempool-policy heap limiter (ConsensusFlags::LIMIT_HEAP) when calling the Rust-native generator (run_block_generator2), and updates various call sites to stop passing that flag.
Changes:
- Add an early error return in
run_block_generator2whenConsensusFlags::LIMIT_HEAPis set. - Update developer tools to use
ConsensusFlags::empty()as the non-mempool baseline flags. - Update tests/fuzz targets that previously passed
MEMPOOL_MODE(transitively includingLIMIT_HEAP) to stripLIMIT_HEAPbefore callingrun_block_generator2.
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/chia-consensus/src/run_block_generator.rs | Reject LIMIT_HEAP in run_block_generator2. |
| crates/chia-consensus/src/test_generators.rs | Strip LIMIT_HEAP from test flag sets before calling run_block_generator2. |
| crates/chia-consensus/src/spendbundle_conditions.rs | Strip LIMIT_HEAP from MEMPOOL_MODE in tests that call run_block_generator2. |
| crates/chia-consensus/src/get_puzzle_and_solution.rs | Strip LIMIT_HEAP from MEMPOOL_MODE in tests that call run_block_generator2. |
| crates/chia-consensus/src/build_compressed_block.rs | Strip LIMIT_HEAP from MEMPOOL_MODE when calling run_block_generator2 in tests. |
| crates/chia-consensus/fuzz/fuzz_targets/run-generator.rs | Stop passing LIMIT_HEAP to run_block_generator2 in fuzzing. |
| crates/chia-tools/src/bin/validate-blockchain-db.rs | Remove unconditional LIMIT_HEAP from tool flags. |
| crates/chia-tools/src/bin/test-block-generators.rs | Switch non-mempool flags from LIMIT_HEAP to empty. |
| crates/chia-tools/src/bin/analyze-chain.rs | Switch non-mempool flags from LIMIT_HEAP to empty. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+278
to
+279
| for (flags, expected) in zip(&[ConsensusFlags::empty(), MEMPOOL_MODE], expected) { | ||
| let mut flags = *flags; | ||
| let mut flags = *flags - ConsensusFlags::LIMIT_HEAP; |
There was a problem hiding this comment.
This line strips LIMIT_HEAP from both iterations, including the MEMPOOL_MODE iteration. Later in the function (inside the UPDATE_TESTS block) the code checks flags.contains(MEMPOOL_MODE) to decide whether to write the STRICT output, but after subtracting LIMIT_HEAP that check will always be false because MEMPOOL_MODE includes LIMIT_HEAP. Consider tracking whether you're in the mempool-mode iteration separately (before removing LIMIT_HEAP), or adjusting the check to match the intended strict flag set.
Comment on lines
+231
to
+236
| if flags.contains(ConsensusFlags::LIMIT_HEAP) { | ||
| return Err(ValidationErr( | ||
| NodePtr::NIL, | ||
| ErrorCode::GeneratorRuntimeError, | ||
| )); | ||
| } |
There was a problem hiding this comment.
The new LIMIT_HEAP rejection is a behavioral change that isn't directly asserted anywhere (most call sites were updated to avoid passing the flag). To prevent regressions, add a focused unit test that calls run_block_generator2 with ConsensusFlags::LIMIT_HEAP and asserts it returns ErrorCode::GeneratorRuntimeError (or whatever error code you intend to expose for this misuse).
![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 OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| NodePtr::NIL, | ||
| ErrorCode::GeneratorRuntimeError, | ||
| )); | ||
| } |
There was a problem hiding this comment.
Fuzz target always panics after LIMIT_HEAP rejection added
High Severity
The new LIMIT_HEAP rejection in run_block_generator2 breaks the run-generator fuzz target, which still passes ConsensusFlags::LIMIT_HEAP on line 24. After this change, run_block_generator2 always immediately returns an error for every fuzz input. When run_block_generator succeeds but run_block_generator2 always fails, the match falls through to the catch-all arm and panics. The fuzz target can never reach the (Ok, Ok) comparison branch, completely defeating its purpose of cross-checking the two generator implementations.
richardkiss
force-pushed
the
limit-heap-error
branch
from
March 11, 2026 22:59
bfc9d7f to
8bcf200
Compare
richardkiss
marked this pull request as draft
March 11, 2026 23:00
richardkiss
added a commit
to richardkiss/chia-blockchain
that referenced
this pull request
Mar 11, 2026
…ting Uses chia_rs@8bcf2006 (Chia-Network/chia_rs#1396) which makes run_block_generator2 return an error if LIMIT_HEAP is set in flags. This PR exists to see which chia-blockchain tests break when LIMIT_HEAP is rejected by the consensus code path. Any test failures indicate sites that pass MEMPOOL_MODE (which includes LIMIT_HEAP) to run_block_generator2 — a separation-of-concerns bug. Adds Rust toolchain setup to the install action so poetry can build chia_rs from source. Made-with: Cursor
richardkiss
force-pushed
the
limit-heap-error
branch
from
March 12, 2026 01:14
8bcf200 to
05be942
Compare
Pull Request Test Coverage Report for Build 23262629980Details
💛 - Coveralls |
LIMIT_HEAP is a mempool-policy flag that caps the CLVM allocator to 500 MB. It has no place in run_block_generator2, which is a consensus code path. The mempool no longer uses run_block_generator2 (confirmed by Arvid, 2026-03-11), so accepting this flag there only muddies the separation of concerns. Changes: - run_block_generator2 now returns GeneratorRuntimeError immediately if ConsensusFlags::LIMIT_HEAP is set in flags. - Developer tools (test-block-generators, analyze-chain, validate-blockchain-db): non-mempool baseline flags changed from LIMIT_HEAP to ConsensusFlags::empty(). These tools run the Rust-native generator and never needed the heap cap. - Tests and fuzz target: strip LIMIT_HEAP from MEMPOOL_MODE before calling run_block_generator2. Test results are unaffected because the test generators don't approach the heap limit. Made-with: Cursor
Made-with: Cursor
These generators use >500 MB of CLVM allocator space when run in MEMPOOL mode with LIMIT_HEAP set, so they previously returned error 117 (GeneratorRuntimeError / heap exhausted) in strict mode. Now that run_block_generator2 rejects LIMIT_HEAP and we strip it from test calls, those generators run to completion and return their actual condition errors (e.g. 14 = AssertHeightAbsoluteFailed). Changes: - spendbundle_conditions.rs: replace get_conditions_from_spendbundle (which hardcodes MEMPOOL_MODE including LIMIT_HEAP) with a direct run_spendbundle call that strips LIMIT_HEAP, so both code paths use the same heap configuration and can be compared. - test_generators.rs: introduce is_mempool boolean so UPDATE_TESTS detection still works correctly after LIMIT_HEAP is stripped from the flags variable. Stripping LIMIT_HEAP made flags.contains(MEMPOOL_MODE) return false, causing UPDATE_TESTS to write the mempool output as a second non-strict line instead of a STRICT section. - generator-tests/*.txt: remove STRICT sections whose only content was FAILED: 117 (heap exhausted). Without LIMIT_HEAP, both mempool and non-mempool passes produce the same condition error, so no STRICT section is needed. Made-with: Cursor
Made-with: Cursor
validate_clvm_and_signature hardcodes make_allocator(LIMIT_HEAP) directly, so LIMIT_HEAP in MEMPOOL_MODE was never doing anything for the real mempool path. Removing it from the constant eliminates the need to strip it manually at every run_block_generator2 call site. The run_block_generator2 guard (return error if LIMIT_HEAP is set) stays as defense-in-depth. Made-with: Cursor
…ly fast only due to OOM Made-with: Cursor
LIMIT_HEAP was introduced as a flag to limit allocator heap size on mempool paths, but all mempool callers (spendbundle_validation, spendbundle_conditions, api.rs) already hardcode it explicitly rather than threading it through as a parameter. The flag was never a true caller-visible switch. Changes: - make_allocator() no longer takes a flags argument; it always returns Allocator::new_limited(500_000_000), which is the correct size for all spend-bundle / mempool paths - Block-validation functions (run_block_generator, run_block_generator2, additions_and_removals, get_coinspends_*) now create their own Allocator::new_limited(u32::MAX) directly, making the intent clear - The LIMIT_HEAP guard in run_block_generator2 is removed; it was only needed to prevent the flag from accidentally imposing a 500 MB cap on block validation, which can no longer happen - LIMIT_HEAP is exported to Python as 0 for backwards compatibility so that existing code doing `flags & ~LIMIT_HEAP` or `flags | LIMIT_HEAP` is a no-op. The flag definition remains in ConsensusFlags (with value 0x0004) only because clvmr requires bit-for-bit parity. - MEMPOOL_MODE still explicitly excludes the LIMIT_HEAP bit to prevent clvmr-level heap enforcement from leaking into block-validation callers that pass MEMPOOL_MODE to run_block_generator2 Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Three places used make_allocator() (500MB) where they should have used an unlimited allocator: 1. spendbundle_conditions tests: convert_block_to_bundle and the a1 allocator in run_generator were comparing against run_block_generator2 (which uses unlimited heap). Large stress-test generators exceeded 500MB, causing the two to diverge and the test to panic. 2. test_generators::run_generator: the per-spend puzzle-runnable check used 500MB, causing assert!(runnable) to fail for many-large-ints generators whose individual puzzles need more than 500MB. 3. run_chia_program: this generic Python-callable CLVM runner should not impose a 500MB cap. Generator performance tests expect programs to hit the cost limit, not an OOM, so they failed with "expected cost to be exceeded". In all three cases the correct allocator is unlimited, consistent with the behavior before make_allocator() was changed to always return 500MB. The 500MB cap belongs exclusively to the mempool spend-bundle paths. Made-with: Cursor
richardkiss
force-pushed
the
limit-heap-error
branch
from
March 18, 2026 19:16
aa5a853 to
7eb04a3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LIMIT_HEAPis a mempool-policy flag that caps the CLVM allocator size. It was originally bundled intoMEMPOOL_MODE(via clvmr'sCLVM_MEMPOOL_MODE), so it flowed into every call site that passedMEMPOOL_MODE— includingrun_block_generator2, which is a consensus code path. Arvid confirmed (2026-03-11) that the mempool no longer usesrun_block_generator2, so there is no reason for a mempool-policy flag to appear there.Mempool transactions arrive as
SpendBundleobjects (already deserialized, individualCoinSpends), so there is no generator to run. The mempool path goes throughrun_spendbundle→validate_clvm_and_signature, which runs each puzzle/solution pair individually.validate_clvm_and_signaturealready hardcodesmake_allocator(ConsensusFlags::LIMIT_HEAP)directly, independent of the flags passed in — soLIMIT_HEAPinMEMPOOL_MODEwas always redundant for the real mempool path.The fix is to drop
LIMIT_HEAPfromMEMPOOL_MODEinflags.rs, making the constant reflect semantic mempool rules (strict args, unknown conditions) rather than bundling in a deployment policy.Changes
flags.rs:MEMPOOL_MODEnow excludesLIMIT_HEAP. Callers that genuinely want heap limiting must add it explicitly (onlyvalidate_clvm_and_signatureneeds it, and it already does so directly).run_block_generator.rs:run_block_generator2returns an error immediately ifLIMIT_HEAPis set — defense-in-depth to make the boundary explicit.build_compressed_block.rs,get_puzzle_and_solution.rs,spendbundle_conditions.rs, tools, fuzz target): Reverted to plainMEMPOOL_MODE— no more- ConsensusFlags::LIMIT_HEAParithmetic.generator-tests/*.txt: RemovedSTRICT:sections that only containedFAILED: 117. These sections existed because mempool mode (withLIMIT_HEAP) caused those generators to OOM before parsing conditions, producing a different error than consensus mode. WithLIMIT_HEAPgone fromMEMPOOL_MODE, both modes fail the same way. TheSTRICT:sections were documenting an accidental OOM difference, not a meaningful semantic difference between mempool and consensus validation.Background
The
STRICT:\nformat in generator test files separates non-mempool expected output from mempool expected output when they differ. A missingSTRICT:section means both modes produce the same result. The removed sections were only everFAILED: 117(heap exhausted) — an artifact ofLIMIT_HEAPbeing inMEMPOOL_MODE, not a real behavioral difference worth preserving.