Repository navigation
fix(blockassembly): keep a mining candidate's coinbase on its job, so a solution without one can be mined - #2027
Merged
freemans13 merged 10 commits intoOct 28, 2026
Conversation
… mine a coinbase-less solution with it A submitminingsolution call without a coinbase could never be mined. Block assembly rebuilt the coinbase with CreateCoinbase, which appends 12 random extranonce bytes, so the merkle root was never the one the miner hashed. The solution failed the proof-of-work check, the job was deleted and block assembly was reset. getminingcandidate with coinbase=true built yet another coinbase in the RPC service that block assembly never saw, and GetCandidateBlock built a third. bitcoin-sv keeps the candidate block, coinbase included, and falls back to that coinbase when a solution omits one; a solution's version goes into the header only. This does the same: - GetMiningCandidate builds the coinbase once per candidate id, stores it on the job and returns it in a new coinbase_tx field of MiningCandidate. Ids repeat across polls, so a held job's coinbase is reused when it pays the same value at the same height, and a mutex makes the read and the store one step so concurrent polls cannot hand out different coinbases. - submitMiningSolution mines a coinbase-less solution with the job's coinbase, unaltered. The version, time and nonce copies into the coinbase are gone. - GetCandidateBlock and getminingcandidate with coinbase=true return that same coinbase. TestSubmitMiningSolution_CoinbaseLessMinesTheIssuedCoinbase mines over a sqlitememory blockchain store in one submit and reads the stored block back. Restoring the rebuild, the reuse or the mutex each turns a test red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-28 12:43 UTC |
freemans13
marked this pull request as ready for review
October 22, 2026 10:40
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: No issues found. The only new commit, 15a33b9, merges upstream/main again. On the paths this PR touches, it changes just two files. In The earlier changes still look correct. A missed share is rejected without a reset, History:
Still not flagged: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…w round 1) Building a candidate's coinbase needs miner_wallet_private_keys, and the previous commit made every getminingcandidate build one, so a node with no keys could no longer serve candidates. A pool that submits its own coinbase and generatetoaddress never needed the keys. A coinbase that cannot be built now leaves the job without one; a coinbase-less solution, GetCandidateBlock and coinbase=true refuse such a job with a message naming the setting, and the refusal does not drop the job. Docs: submitminingsolution's coinbase is optional, and provideCoinbaseTx returns the coinbase kept on the job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc
Collaborator
Author
|
Reply to the Claude Code Review of 6ab204a, both points fixed in ecb0026.
|
# Conflicts: # CHANGELOG.md
GenerateBlocks with an explicit P2PKH address and miner_wallet_private_keys cleared must mine a block: the best height advances by one and the stored block's coinbase pays that address. The test fails if storeJobWithCoinbase goes back to failing the candidate when the job's own coinbase cannot be built. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc
ordishs
approved these changes
Oct 22, 2026
ordishs
left a comment
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Approved. The coinbase is built once per candidate, stored under a mutex, and mined unaltered on submit. This matches bitcoin-sv. The tests assert exact bytes against a real store and cover the race.
Two non-blocking notes:
services/blockassembly/Server.go(storeJobWithCoinbase): a failure to build the coinbase logs at Debug level only. An invalid WIF inminer_wallet_private_keysthen looks the same as "no keys". Consider a Warn when keys are configured but the build fails.MiningCandidategainscoinbase_tx(field 13). Does the Rust BlockAssembly port build and return it? If not,getminingcandidate coinbase=trueerrors against that service.
icellan
reviewed
Oct 23, 2026
icellan
left a comment
icellan
left a comment
Contributor
There was a problem hiding this comment.
Two issues, neither blocking. Details inline.
…nd warn on unusable miner keys A consensus-invalid submitminingsolution deleted the job. With the coinbase now kept on the job and candidate ids repeating (the empty-block id is derived from the parent alone), the next poll built a coinbase with a fresh extranonce, so every other coinbase-less miner on that id failed proof of work and deleted the job again. The ErrBlockInvalid branch now keeps the job, as bitcoin-sv leaves a candidate alone after an invalid solution; the TTL and the mined-block DeleteAll still clear it. That delete came from e787b23 (upstream PR 1748); this PR's coinbase reuse is what made it harmful. A share that misses the target never reached that branch: Block.Valid reports a miss as a processing error (HasMetTargetDifficulty returns one with the miss, so the BlockInvalid line after it is unreachable), which took the reset path, reset block assembly and dropped the job. submitMiningSolution now checks the target itself before Block.Valid and rejects a miss without a reset, keeping the job. The reset path and its delete are unchanged for faults not attributable to the submitter; Block.Valid is untouched, so peer block validation is unaffected. storeJobWithCoinbase logged every coinbase build failure at Debug, so a bad WIF in miner_wallet_private_keys looked like no keys. With a non-blank key configured it now warns with the cause, at most once a minute; with no keys it stays at Debug. The getminingcandidate coinbase=true error now also names an implementation that does not set coinbase_tx. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc
Collaborator
Author
|
@ordishs, thanks for the review. Answers to your two notes: 1. Bad WIF logged at Debug. Fixed in b44c4b2, the same fix as icellan's inline comment on 2. The Rust BlockAssembly port and
Where does the Rust port live? If you point me at it, I will check whether it builds and returns the candidate's coinbase, or raise an issue there to add field 13. |
withIssuedCoinbase was inserted between requirePastFloorGuard and its doc comment, so godoc attached that comment to the wrong function. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc
icellan
approved these changes
Oct 27, 2026
…e-keeps-coinbase # Conflicts: # CHANGELOG.md
|
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.
What happened
A
submitminingsolutioncall with nocoinbasecould never produce a block. Block assembly rebuilt the coinbase on submit withCreateCoinbase, which appends 12 random extranonce bytes, so the merkle root never matched the one the miner hashed. Every such solution failed the proof-of-work check, deleted the job and reset block assembly. icellan found this reviewing #2017.Three places each built their own coinbase for the same job, and none of them agreed:
GetCandidateBlockbuilt another;getminingcandidatewithcoinbase=truebuilt a third in the RPC service, a version 2 pay-to-pubkey transaction that block assembly never saw.bitcoin-sv keeps the whole candidate block. When a solution omits its coinbase it falls back to the candidate's own (
result->GetBlockCoinbase()insrc/rpc/mining-fbb.cpp), and a solution'sversionchanges only the header.Changes
GetMiningCandidatebuilds the coinbase once and stores it on the job. It also returns it in a newcoinbase_txfield on theMiningCandidateproto, so the RPC service can hand out the same bytes.GetCandidateBlockandgetminingcandidate coinbase=truereturn the same coinbase. The RPC now returns the job's version 1 P2PKH coinbase instead of building a pay-to-pubkey one. If block assembly sends no coinbase, the RPC returns an error rather than inventing one.Testing
TestSubmitMiningSolution_CoinbaseLessMinesTheIssuedCoinbaseissues a candidate over a sqlitememory blockchain store holding a real chain. It mines the header over the issued coinbase with a version override, submits once with no coinbase and no retries, then reads the stored block back. The block carries the issued coinbase byte for byte, and the version is in the header.TestGetCandidateBlock_CarriesTheIssuedCoinbasechecks that the served candidate block has the same coinbase.TestStoreJobWithCoinbase_OncePerCandidateIDcovers three cases: a repeated id reuses the coinbase, 16 concurrent polls of one id all get the held coinbase (run under-race), and a changed payout gets a new coinbase.go test -racepasses forservices/blockassembly/...,services/rpc/...andmodel/....golangci-lint --new-from-rev=upstream/mainreports no issues.Limits and trade-offs
getminingcandidate coinbase=trueerrors until block assembly is upgraded. I chose the error over a fallback. A fallback would hand out a coinbase that block assembly never sees, which is the bug.MiningCandidatenow carries the coinbase, about 200 bytes per poll.mining.Mine(thegeneratepath) still builds its own coinbase. That is fine, because it always submits it.submitMiningSolution, removing the time and nonce copies. Whichever merges second will conflict there, and the resolution is this PR's version. fix(blockvalidation): reject blocks whose coinbase is not final, and stop block assembly mining them #2017's recreate test has a retry loop that only exists because of the random rebuild; once both are in it should become a single submit.model.pb.gowas regenerated with a slightly newer protoc (v7.36.2, protoc-gen-go v1.36.12) than it was last generated with, so its version header lines changed.nodebinary is the wrong CPU type), so the CHANGELOG entry was checked against the neighbouring entries' format by hand.🤖 Generated with Claude Code
https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz