Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33421. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
ismaelsadeeq
force-pushed
the
09-2025-minerman
branch
2 times, most recently
from
9682ea7 to
dca8200
Compare
ismaelsadeeq
force-pushed
the
09-2025-minerman
branch
from
dca8200 to
16d5558
Compare
|
Forced pushed from dca8200c71bb568ffe4821c68209e07a305db80d to 16d55585aa2d26e9177734927b8446faa72570e7 5db80d..16d5558 Main Changes are
|
|
It seems the new fuzz test in 16d5558 https://github.com/bitcoin/bitcoin/actions/runs/17949962995/job/51047356983?pr=33421 This is possible on master when node is started with This undefined behaviour is possible after #32355 because it introduced a fixed I'll push a commit with a fix soon. |
ismaelsadeeq
force-pushed
the
09-2025-minerman
branch
from
16d5558 to
2dffe16
Compare
glozow
added
Bug
Needs backport (29.x)
Needs backport (30.x)
and removed
Bug
Needs backport (29.x)
Needs backport (30.x)
labels
…rflow b807dfc miner: fix `addPackageTxs` unsigned integer overflow (ismaelsadeeq) Pull request description: This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`. The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty. i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`. Instead of bailing out after going through transactions equivalent to `MAX_CONSECUTIVE_FAILURES`, the loop never breaks until all mempool transactions are visited. See #33421 (comment) The fix avoids the overflow by using addition instead adding `BLOCK_FULL_ENOUGH_WEIGHT_DELTA` to the block weight and comparing it with `m_options.nBlockMaxWeight`. Another alternative that preserves the same structure is to use `static_cast`. See c9530cf. This fix can be tested by cherry-picking the commits from #33421 without the static cast fix and running: ```bash echo "AQAAAAAAA AAnJycnAAAAAAAAAAAAAAAAAA" | base64 --decode > miner.crash FUZZ=block_template_cache ./build_fuzz/bin/fuzz miner.crash ``` --- This is part of a larger inconsistency in how size/weight is represented in the codebase. It may be worth defining a dedicated type for size/weight. ACKs for top commit: glozow: nice, utACK b807dfc furszy: Code ACK b807dfc Tree-SHA512: c1d2f7e500f9b0624a4c22a146921a1644017065e6c94d0c5027486392321f5de26c61751a24765e025e45b34c535adfd6d0e2ac809dea6846b99f37d13043c9
ismaelsadeeq
force-pushed
the
09-2025-minerman
branch
from
2dffe16 to
ed482de
Compare
ismaelsadeeq
force-pushed
the
09-2025-minerman
branch
from
66f3244 to
90eaf64
Compare
ismaelsadeeq
force-pushed
the
09-2025-minerman
branch
3 times, most recently
from
58a7be5 to
ae737b9
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
ismaelsadeeq
force-pushed
the
09-2025-minerman
branch
from
ae737b9 to
1f623ca
Compare
| * Search for a cached template matching the provided options. | ||
| * | ||
| * - If found and within the age limit, return it. | ||
| * - If found but too old, evict it and return nullptr. |
There was a problem hiding this comment.
In 4619759 miner: introduce BlockTemplateCache for reusing block templates: this is somewhat surprising behavior in isolation. When you have two clients, one client might want a recent template and the second client allows an older one.
But it makes sense because an evicted template is guaranteed to be replaced.
I tried some alternative approaches, but it was ugly.
Maybe rename to getOrDeleteCachedTemplate?
There was a problem hiding this comment.
This is okay; it is a shared pointer, so evicting here does not affect a client who holds onto an older one.
- This prevents circular dependecy when we introduce BlockTemplateCache in miner and want to expose it via the node context.
|
Forced pushed from 1f623ca to e413103
C.I failure seems unrelated. I opened #34716 to track it. |
Because the CBlockTemplate struct now has a mockable steady clock variable, all fuzz tests that invoke code paths that create a CBlockTemplate will now emit a warning if a SteadyClockContext is not explicitly initialized. Hence, this commit initializes a SteadyClockContext for all affected fuzz tests.
Co-authored-by: Ryan Ofsky <[email protected]>
Co-authored-by: Ryan Ofsky <[email protected]>
|
Forced pushed from e413103 to 20fc737 compare diff This push addressed review comments from @ryanofsky
|
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 20fc737. Mainly max_age corner case cleanup since last review and other simplifications. The code seem very simple and clean now. Left a few more suggestions, but definitely feel free to ignore them and avoid another update. This looks very good
| static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(); | ||
| g_setup = testing_setup.get(); | ||
| SetMockTime(WITH_LOCK(g_setup->m_node.chainman->GetMutex(), return g_setup->m_node.chainman->ActiveTip()->Time())); | ||
|
|
There was a problem hiding this comment.
In commit "miner: add block creation time point to CBlockTemplate" (9b6475a)
MIght be an unintended whitespace change here
There was a problem hiding this comment.
yeah will do if I need to retouch.
| /* Uses steady clock because it is monotonic and unaffected by rare system clock changes. | ||
| * It uses the mockable version to allow for testing. | ||
| * Used to decide whether a cached template is eligible for reuse based on its age.*/ | ||
| MockableSteadyClock::time_point m_creation_time; |
There was a problem hiding this comment.
In commit "miner: add block creation time point to CBlockTemplate" (9b6475a)
Would it make sense to assign a default value like m_creation_time{MockableSteadyClock::now()}?
If not, it may be better to at least initialize with m_creation_time{} so the value will be 0 instead of an indeterminate value.
There was a problem hiding this comment.
Would it make sense to assign a default value like m_creation_time{MockableSteadyClock::now()}?
If not, it may be better to at least initialize with m_creation_time{} so the value will be 0 instead of an indeterminate value.
Hmm, I'd rather leave it uninitialized for now (no code path instantiates a CBlockTemplate except via CreateNewBlock). A missed initialization should ideally be a detectable bug rather than a silent default value.
So this is a good flag, but I'd fix this by having an explicit constructor that accepts all CBlockTemplate struct variables so that no field can be left unassigned.
Leaving as-is for now; no degradation, it's the same assumption made with CBlock and the other values in the struct.
| m_block_templates.erase(it); | ||
| return nullptr; | ||
| } | ||
| return it->block_template; |
There was a problem hiding this comment.
In commit "miner: introduce BlockTemplateCache for reusing block templates" (c3f106c)
Maybe do return Assert(it->block_template) to be clear this pointer is never expected to be null
There was a problem hiding this comment.
when retouching, I will add the assert.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
7 participants
This PR implements the first step of #33758 by adding a cache layer for node block templates.
The cache stores newly created block templates along with their configuration options. When clients request a block template, they specify an optional template age limit in
MillisecondsDouble:nullopt, we create a new template and return it without adding it to the cache.The cache maintains a configurable maximum size (default: 10 templates). When this limit is exceeded, the oldest template is evicted after each insertion.
Cache lookup requires an exact match on ALL
BlockAssembler::Optionsfields. While some fields (e.g.,test_block_validity,print_modified_fee,coinbase_output_script) could be ignored for cache reuse, they mainly differ in tests and benchmarks. We keep the logic simple and accept cache misses in those cases.The cache enforces at most one template per set of options. When searching for a match in
getCachedTemplate(), if a template with matching options is found but has exceeded the age limit, it's immediately evicted before returningnullptr. This ensures no duplicate options exist in the cache. ASanityCheck()method verifies this invariant in tests by checking all cached entries for duplicates.The cache subscribes to the validation interface and clears itself when blocks are connected (to non-historical chainstate to avoid clearing during assumeutxo background validation) or disconnected from the active chain (during reorgs), preventing stale or invalid templates from being served.
GetBlockTemplate()uses double-checked locking: it first checks the cache with onlym_mutexheld, then if there's a miss, acquires bothcs_mainandm_mutexto create the template. Before creating, it checks again since another thread might have added a template while locks were released.This PR removes an unused
node/context.himport from miner to prevent a circular dependency when exposing the block template cache to the node context.