From 682def3dc084ab74095306063f3983d3ffc26b1d Mon Sep 17 00:00:00 2001 From: freemans13 Date: Thu, 22 Oct 2026 11:01:18 +0100 Subject: [PATCH 1/6] fix(blockassembly): keep a mining candidate's coinbase on its job and 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 Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz --- CHANGELOG.md | 16 ++ model/model.pb.go | 27 +- model/model.proto | 4 + services/blockassembly/Server.go | 80 ++++-- .../blockassembly/candidate_coinbase_test.go | 243 ++++++++++++++++++ .../submit_mining_solution_test.go | 26 +- services/rpc/handlers.go | 11 +- services/rpc/handlers_additional_test.go | 14 +- 8 files changed, 374 insertions(+), 47 deletions(-) create mode 100644 services/blockassembly/candidate_coinbase_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a96be442b..77ce4f3ca9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,6 +106,22 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea ### Breaking — behaviour changes on upgrade +- **A mining candidate keeps its coinbase, and a solution without one is mined with it.** + Affects miners and pools that use `getminingcandidate` and `submitminingsolution`. As in + bitcoin-sv, block assembly now builds a candidate's coinbase once, keeps it on the job, + and returns it in the new `coinbase_tx` field of the `MiningCandidate` message. A + `submitminingsolution` call with no `coinbase` is mined with that coinbase. Before, block + assembly rebuilt one with a fresh random extranonce, so its merkle root was never the one + the miner hashed and every such solution failed the proof-of-work check, deleted the job + and reset block assembly. A solution's `version` now goes into the header only, no longer + into the coinbase as well. `getminingcandidate` with `coinbase=true` returns the kept + coinbase: a version 1 transaction paying the configured miner wallet addresses by P2PKH, + where it used to return a separately built version 2 transaction with one pay-to-pubkey + output that block assembly never saw. Miners that submit their own coinbase are not + affected. Upgrade block assembly no later than the RPC service: an RPC service on this + build answers `getminingcandidate` with `coinbase=true` with an error while it is talking + to an older block assembly, which sends no coinbase. (#TBD) + - **`utxostore_updateTxMinedStatus` is removed, and it was live.** Affects only nodes that set it to `false`. Mined-tagging, the step after `AddBlock` that writes a block's ID onto each of its transactions, now always runs. So does the check it carries, which rejects a diff --git a/model/model.pb.go b/model/model.pb.go index 5fd2bb51ca..1070e73313 100644 --- a/model/model.pb.go +++ b/model/model.pb.go @@ -1,7 +1,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 -// protoc v7.34.1 +// protoc-gen-go v1.36.12 +// protoc v7.36.2 // source: model/model.proto package model @@ -105,8 +105,12 @@ type MiningCandidate struct { NumTxs uint32 `protobuf:"varint,10,opt,name=num_txs,json=numTxs,proto3" json:"num_txs,omitempty"` SizeWithoutCoinbase uint64 `protobuf:"varint,11,opt,name=size_without_coinbase,json=sizeWithoutCoinbase,proto3" json:"size_without_coinbase,omitempty"` SubtreeHashes [][]byte `protobuf:"bytes,12,rep,name=subtree_hashes,json=subtreeHashes,proto3" json:"subtree_hashes,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + // The coinbase block assembly built for this candidate and keeps on its job. A solution that + // omits its own coinbase is mined with this one, as in bitcoin-sv, so it is the coinbase a miner + // must hash when it does not supply one. + CoinbaseTx []byte `protobuf:"bytes,13,opt,name=coinbase_tx,json=coinbaseTx,proto3" json:"coinbase_tx,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *MiningCandidate) Reset() { @@ -223,6 +227,13 @@ func (x *MiningCandidate) GetSubtreeHashes() [][]byte { return nil } +func (x *MiningCandidate) GetCoinbaseTx() []byte { + if x != nil { + return x.CoinbaseTx + } + return nil +} + // swagger:model MiningSolution type MiningSolution struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -311,7 +322,7 @@ func (x *MiningSolution) GetBlockHash() []byte { // swagger:model NotificationMetadata type NotificationMetadata struct { state protoimpl.MessageState `protogen:"open.v1"` - // define a map of string to string + //define a map of string to string Metadata map[string]string `protobuf:"bytes,1,rep,name=metadata,proto3" json:"metadata,omitempty" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"bytes,2,opt,name=value"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -818,7 +829,7 @@ var File_model_model_proto protoreflect.FileDescriptor const file_model_model_proto_rawDesc = "" + "\n" + - "\x11model/model.proto\x12\x05model\x1a\x1fgoogle/protobuf/timestamp.proto\"\x84\x03\n" + + "\x11model/model.proto\x12\x05model\x1a\x1fgoogle/protobuf/timestamp.proto\"\xa5\x03\n" + "\x0fMiningCandidate\x12\x0e\n" + "\x02id\x18\x01 \x01(\fR\x02id\x12#\n" + "\rprevious_hash\x18\x02 \x01(\fR\fpreviousHash\x12%\n" + @@ -832,7 +843,9 @@ const file_model_model_proto_rawDesc = "" + "\anum_txs\x18\n" + " \x01(\rR\x06numTxs\x122\n" + "\x15size_without_coinbase\x18\v \x01(\x04R\x13sizeWithoutCoinbase\x12%\n" + - "\x0esubtree_hashes\x18\f \x03(\fR\rsubtreeHashes\"\xbd\x01\n" + + "\x0esubtree_hashes\x18\f \x03(\fR\rsubtreeHashes\x12\x1f\n" + + "\vcoinbase_tx\x18\r \x01(\fR\n" + + "coinbaseTx\"\xbd\x01\n" + "\x0eMiningSolution\x12\x0e\n" + "\x02id\x18\x01 \x01(\fR\x02id\x12\x1a\n" + "\bcoinbase\x18\x02 \x01(\fR\bcoinbase\x12\x17\n" + diff --git a/model/model.proto b/model/model.proto index c881809ea0..18c4574c79 100644 --- a/model/model.proto +++ b/model/model.proto @@ -20,6 +20,10 @@ message MiningCandidate { uint32 num_txs = 10; uint64 size_without_coinbase = 11; repeated bytes subtree_hashes = 12; + // The coinbase block assembly built for this candidate and keeps on its job. A solution that + // omits its own coinbase is mined with this one, as in bitcoin-sv, so it is the coinbase a miner + // must hash when it does not supply one. + bytes coinbase_tx = 13; } // swagger:model MiningSolution diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index 4189ef878c..2c7fd2ebd1 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -110,6 +110,11 @@ type BlockAssembly struct { // jobStore caches mining jobs with TTL jobStore *ttlcache.Cache[chainhash.Hash, *subtreeprocessor.Job] + // jobCoinbaseMu makes GetMiningCandidate's read of a job's coinbase and its store of the new + // job one step, so two polls that share a candidate id cannot each build a coinbase and leave + // one miner holding a coinbase the job no longer has. + jobCoinbaseMu sync.Mutex + // blockSubmissionChan handles block submission requests blockSubmissionChan chan *BlockSubmissionRequest @@ -1668,11 +1673,9 @@ func (ba *BlockAssembly) GetMiningCandidate(ctx context.Context, req *blockassem id, _ := chainhash.NewHash(miningCandidate.Id) - ba.jobStore.Set(*id, &subtreeprocessor.Job{ - ID: id, - Subtrees: subtrees, - MiningCandidate: miningCandidate, - }, jobTTL) // create a new job with a TTL, will be cleaned up automatically + if err = ba.storeJobWithCoinbase(id, miningCandidate, subtrees); err != nil { + return nil, errors.WrapGRPC(err) + } if includeSubtreeHashes { miningCandidate.SubtreeHashes = make([][]byte, len(subtrees)) @@ -1691,6 +1694,42 @@ func (ba *BlockAssembly) GetMiningCandidate(ctx context.Context, req *blockassem return miningCandidate, nil } +// storeJobWithCoinbase gives the candidate its coinbase and stores it as a job with a TTL, after +// which it is cleaned up automatically. The coinbase is the one a solution without its own is mined +// with, as bitcoin-sv keeps the candidate block's coinbase, so it must not change under a miner who +// has been handed it. Candidate ids repeat: the main path hashes the subtree root, parent and time, +// so polls in the same second share one, and the empty-block path derives its id from the parent +// alone. A poll for an id whose job is still held therefore reuses that job's coinbase rather than +// building a new one, provided it pays the same value at the same height. +func (ba *BlockAssembly) storeJobWithCoinbase(id *chainhash.Hash, miningCandidate *model.MiningCandidate, subtrees []*subtreepkg.Subtree) error { + ba.jobCoinbaseMu.Lock() + defer ba.jobCoinbaseMu.Unlock() + + if held := ba.jobStore.Get(*id); held != nil { + prev := held.Value().MiningCandidate + if len(prev.CoinbaseTx) > 0 && prev.CoinbaseValue == miningCandidate.CoinbaseValue && prev.Height == miningCandidate.Height { + miningCandidate.CoinbaseTx = prev.CoinbaseTx + } + } + + if len(miningCandidate.CoinbaseTx) == 0 { + coinbaseTx, err := miningCandidate.CreateCoinbaseTxCandidate(ba.blockAssembler.settings) + if err != nil { + return errors.NewProcessingError("[GetMiningCandidate][%s] failed to create coinbase tx", id.String(), err) + } + + miningCandidate.CoinbaseTx = coinbaseTx.Bytes() + } + + ba.jobStore.Set(*id, &subtreeprocessor.Job{ + ID: id, + Subtrees: subtrees, + MiningCandidate: miningCandidate, + }, jobTTL) + + return nil +} + // SubmitMiningSolution processes a mining solution submission. // It validates the solution, creates a block, and adds it to the blockchain. // @@ -1934,23 +1973,15 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub return nil, errors.NewProcessingError("[BlockAssembly][%s] submitted coinbase transaction is not a valid coinbase", jobID) } } else { - // recreate coinbase tx here, nothing was passed in - coinbaseTx, err = jobItem.Value().MiningCandidate.CreateCoinbaseTxCandidate(ba.blockAssembler.settings) + // No coinbase was passed in, so mine the one the job was issued with, which is the one + // GetMiningCandidate handed out, as bitcoin-sv falls back to the candidate block's coinbase. + // It used to be rebuilt here, with a fresh random extranonce, so its merkle root was never + // the one the miner hashed and every such solution failed the proof-of-work check. The + // solution's version, time and nonce belong in the header, built from req below, and are not + // copied into the coinbase, so it is used exactly as issued. + coinbaseTx, err = bt.NewTxFromBytes(job.MiningCandidate.CoinbaseTx) if err != nil { - return nil, errors.NewProcessingError("[BlockAssembly][%s] failed to create coinbase tx", jobID, err) - } - - // set the new mining parameters on the coinbase (nonce) - if req.Version != nil { - coinbaseTx.Version = *req.Version - } - - if req.Time != nil { - coinbaseTx.LockTime = *req.Time - } - - if req.Nonce != 0 { - coinbaseTx.Inputs[0].SequenceNumber = req.Nonce + return nil, errors.NewProcessingError("[BlockAssembly][%s] job holds no usable coinbase tx", jobID, err) } } @@ -2290,10 +2321,11 @@ func (ba *BlockAssembly) GetCandidateBlock(ctx context.Context, req *blockassemb job := jobItem.Value() - // Create default coinbase transaction from the mining candidate - coinbaseTx, err := job.MiningCandidate.CreateCoinbaseTxCandidate(ba.blockAssembler.settings) + // The coinbase the job was issued with, so this block is the one a coinbase-less solution to the + // candidate would mine. + coinbaseTx, err := bt.NewTxFromBytes(job.MiningCandidate.CoinbaseTx) if err != nil { - return nil, errors.WrapGRPC(errors.NewProcessingError("[GetCandidateBlock][%s] failed to create coinbase tx", candidateID, err)) + return nil, errors.WrapGRPC(errors.NewProcessingError("[GetCandidateBlock][%s] job holds no usable coinbase tx", candidateID, err)) } coinbaseTxIDHash := coinbaseTx.TxIDChainHash() diff --git a/services/blockassembly/candidate_coinbase_test.go b/services/blockassembly/candidate_coinbase_test.go new file mode 100644 index 0000000000..15d6baff67 --- /dev/null +++ b/services/blockassembly/candidate_coinbase_test.go @@ -0,0 +1,243 @@ +package blockassembly + +import ( + "context" + "net/url" + "sync" + "testing" + "time" + + "github.com/bsv-blockchain/go-bt/v2" + "github.com/bsv-blockchain/go-bt/v2/chainhash" + "github.com/bsv-blockchain/go-chaincfg" + "github.com/bsv-blockchain/teranode/model" + "github.com/bsv-blockchain/teranode/services/blockassembly/blockassembly_api" + "github.com/bsv-blockchain/teranode/services/blockchain" + blockchainstore "github.com/bsv-blockchain/teranode/stores/blockchain" + "github.com/bsv-blockchain/teranode/ulogger" + "github.com/bsv-blockchain/teranode/util" + "github.com/bsv-blockchain/teranode/util/testutil" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/proto" +) + +// candidateCoinbaseHarness is a block assembly server over a real blockchain store: a sqlitememory +// store behind the ordinary LocalClient, holding a short regtest chain the candidate builds on. The +// server adds a mined block to that store, so a test reads back what was actually mined. +type candidateCoinbaseHarness struct { + server *BlockAssembly + store blockchain.ClientI + tip *model.BlockHeader + candidate *model.MiningCandidate + id *chainhash.Hash +} + +func newCandidateCoinbaseHarness(t *testing.T) *candidateCoinbaseHarness { + t.Helper() + + common := testutil.NewCommonTestSetup(t) + + tSettings := *common.Settings + params := chaincfg.RegressionNetParams + tSettings.ChainCfgParams = ¶ms + + storeURL, err := url.Parse("sqlitememory://") + require.NoError(t, err) + + store, err := blockchainstore.NewStore(ulogger.TestLogger{}, storeURL, &tSettings) + require.NoError(t, err) + + client, err := blockchain.NewLocalClient(ulogger.TestLogger{}, &tSettings, store, nil, nil) + require.NoError(t, err) + + tip := buildFutureChain(t, t.Context(), client, uint32(time.Now().Add(-time.Hour).Unix())) //nolint:gosec + + _, tipMeta, err := client.GetBlockHeader(t.Context(), tip.Hash()) + require.NoError(t, err) + + s := New(common.Logger, &tSettings, nil, nil, nil, client) + s.blockAssembler = &BlockAssembler{settings: &tSettings, logger: common.Logger, resetCh: make(chan resetRequest, 4)} + + t.Cleanup(func() { _ = s.Stop(context.Background()) }) + + // A best block whose parent differs from the candidate's, so the candidate is not stale. + s.blockAssembler.bestBlock.Store(&BestBlockInfo{ + Header: &model.BlockHeader{HashPrevBlock: &chainhash.Hash{7}}, + Height: tipMeta.Height, + }) + + nBits, err := model.NewNBitFromString("207fffff") + require.NoError(t, err) + + height := tipMeta.Height + 1 + + candidate := &model.MiningCandidate{ + Id: chainhash.HashB(tip.Hash()[:]), + PreviousHash: tip.Hash().CloneBytes(), + CoinbaseValue: util.GetBlockSubsidyForHeight(height, ¶ms), + Version: miningCandidateVersion, + NBits: nBits.CloneBytes(), + Time: tip.Timestamp + 1, + Height: height, + MerkleProof: [][]byte{}, + } + + id, err := chainhash.NewHash(candidate.Id) + require.NoError(t, err) + + return &candidateCoinbaseHarness{server: s, store: client, tip: tip, candidate: candidate, id: id} +} + +// issue stores the candidate's job the way GetMiningCandidate does, and returns a copy of the +// candidate as the miner received it. +func (h *candidateCoinbaseHarness) issue(t *testing.T) *model.MiningCandidate { + t.Helper() + + require.NoError(t, h.server.storeJobWithCoinbase(h.id, h.candidate, nil)) + require.NotEmpty(t, h.candidate.CoinbaseTx, "a stored job must carry its coinbase") + + issued := proto.Clone(h.candidate).(*model.MiningCandidate) + + return issued +} + +// mine finds a nonce for the header a miner builds from the issued candidate: the candidate's own +// coinbase, which with no subtrees is the whole merkle root, under the given version. +func mine(t *testing.T, issued *model.MiningCandidate, version uint32) uint32 { + t.Helper() + + coinbase, err := bt.NewTxFromBytes(issued.CoinbaseTx) + require.NoError(t, err) + + prev, err := chainhash.NewHash(issued.PreviousHash) + require.NoError(t, err) + + nBits, err := model.NewNBitFromSlice(issued.NBits) + require.NoError(t, err) + + header := &model.BlockHeader{ + Version: version, + HashPrevBlock: prev, + HashMerkleRoot: coinbase.TxIDChainHash(), + Timestamp: issued.Time, + Bits: *nBits, + } + + for { + if ok, _, _ := header.HasMetTargetDifficulty(); ok { + return header.Nonce + } + + header.Nonce++ + } +} + +// TestSubmitMiningSolution_CoinbaseLessMinesTheIssuedCoinbase pins bitcoin-sv's submitminingsolution +// fallback: a solution with no coinbase is mined with the coinbase the candidate was issued with. +// Block assembly used to rebuild one with a fresh random extranonce, so its merkle root was never +// the one the miner hashed and every such solution failed the proof-of-work check. A version in the +// solution goes into the header only, as in bitcoin-sv; it used to be copied into the coinbase too. +// One submit, no retries: the miner hashes exactly the block the server builds. +func TestSubmitMiningSolution_CoinbaseLessMinesTheIssuedCoinbase(t *testing.T) { + h := newCandidateCoinbaseHarness(t) + issued := h.issue(t) + + version := uint32(0x20000004) + nonce := mine(t, issued, version) + + resp, err := h.server.submitMiningSolution(t.Context(), &BlockSubmissionRequest{ + SubmitMiningSolutionRequest: &blockassembly_api.SubmitMiningSolutionRequest{ + Id: issued.Id, + Nonce: nonce, + Version: &version, + }, + }) + require.NoError(t, err) + require.True(t, resp.Ok) + + best, _, err := h.store.GetBestBlockHeader(t.Context()) + require.NoError(t, err) + require.Equal(t, h.tip.Hash(), best.HashPrevBlock, "the block must be stored on the candidate's parent") + + block, err := h.store.GetBlock(t.Context(), best.Hash()) + require.NoError(t, err) + require.Equal(t, issued.CoinbaseTx, block.CoinbaseTx.Bytes(), "the block must carry the coinbase the candidate was issued with") + require.Equal(t, version, block.Header.Version, "the solution's version belongs in the header") + require.Equal(t, nonce, block.Header.Nonce) +} + +// TestGetCandidateBlock_CarriesTheIssuedCoinbase pins that the candidate block served for a job is +// built from the same coinbase the job was issued with, not one built on the spot. +func TestGetCandidateBlock_CarriesTheIssuedCoinbase(t *testing.T) { + h := newCandidateCoinbaseHarness(t) + issued := h.issue(t) + + resp, err := h.server.GetCandidateBlock(t.Context(), &blockassembly_api.GetCandidateBlockRequest{Id: issued.Id}) + require.NoError(t, err) + require.Equal(t, issued.CoinbaseTx, resp.CoinbaseTx) +} + +// TestStoreJobWithCoinbase_OncePerCandidateID pins that a candidate id keeps one coinbase for as long +// as its job is held. Ids repeat across polls, so building a coinbase per poll would leave an earlier +// miner holding a coinbase the job no longer has. +func TestStoreJobWithCoinbase_OncePerCandidateID(t *testing.T) { + t.Run("a repeated id reuses the held coinbase", func(t *testing.T) { + h := newCandidateCoinbaseHarness(t) + first := h.issue(t) + + again := proto.Clone(h.candidate).(*model.MiningCandidate) + again.CoinbaseTx = nil + + require.NoError(t, h.server.storeJobWithCoinbase(h.id, again, nil)) + require.Equal(t, first.CoinbaseTx, again.CoinbaseTx) + require.Equal(t, first.CoinbaseTx, h.server.jobStore.Get(*h.id).Value().MiningCandidate.CoinbaseTx) + }) + + t.Run("concurrent polls of one id agree on its coinbase", func(t *testing.T) { + // Two candidate paths and many miners poll at once. Run under -race. + h := newCandidateCoinbaseHarness(t) + + const polls = 16 + + got := make([][]byte, polls) + + var wg sync.WaitGroup + + for i := range polls { + wg.Add(1) + + go func() { + defer wg.Done() + + poll := proto.Clone(h.candidate).(*model.MiningCandidate) + poll.CoinbaseTx = nil + + require.NoError(t, h.server.storeJobWithCoinbase(h.id, poll, nil)) + got[i] = poll.CoinbaseTx + }() + } + + wg.Wait() + + held := h.server.jobStore.Get(*h.id).Value().MiningCandidate.CoinbaseTx + for i := range polls { + require.Equal(t, held, got[i], "poll %d was handed a coinbase the job does not hold", i) + } + }) + + t.Run("a different coinbase value gets a new coinbase", func(t *testing.T) { + h := newCandidateCoinbaseHarness(t) + first := h.issue(t) + + changed := proto.Clone(h.candidate).(*model.MiningCandidate) + changed.CoinbaseTx = nil + changed.CoinbaseValue-- + + require.NoError(t, h.server.storeJobWithCoinbase(h.id, changed, nil)) + require.NotEqual(t, first.CoinbaseTx, changed.CoinbaseTx) + + coinbase, err := bt.NewTxFromBytes(changed.CoinbaseTx) + require.NoError(t, err) + require.Equal(t, changed.CoinbaseValue, coinbase.TotalOutputSatoshis()) + }) +} diff --git a/services/blockassembly/submit_mining_solution_test.go b/services/blockassembly/submit_mining_solution_test.go index 512acc3b8d..9c3105e575 100644 --- a/services/blockassembly/submit_mining_solution_test.go +++ b/services/blockassembly/submit_mining_solution_test.go @@ -14,6 +14,7 @@ import ( "github.com/bsv-blockchain/teranode/services/blockassembly/blockassembly_api" "github.com/bsv-blockchain/teranode/services/blockassembly/subtreeprocessor" "github.com/bsv-blockchain/teranode/services/blockchain" + "github.com/bsv-blockchain/teranode/settings" "github.com/bsv-blockchain/teranode/ulogger" "github.com/bsv-blockchain/teranode/util/testutil" "github.com/stretchr/testify/mock" @@ -481,6 +482,19 @@ func TestSubmitMiningSolution_NoCurrentBlock(t *testing.T) { // downstream failure keeps it honest: if the fixture stops dying exactly there, // this fails loudly and whoever changed it re-checks that the guard is still on // the path being exercised. +// withIssuedCoinbase gives a hand-built candidate the coinbase GetMiningCandidate would have stored +// on its job, which a solution without its own coinbase is mined with. +func withIssuedCoinbase(t *testing.T, tSettings *settings.Settings, candidate *model.MiningCandidate) *model.MiningCandidate { + t.Helper() + + coinbaseTx, err := candidate.CreateCoinbaseTxCandidate(tSettings) + require.NoError(t, err) + + candidate.CoinbaseTx = coinbaseTx.Bytes() + + return candidate +} + func requirePastFloorGuard(t *testing.T, err error) { t.Helper() @@ -537,11 +551,11 @@ func TestSubmitMiningSolution_NTimeFloor(t *testing.T) { s.jobStore.Set(*id, &subtreeprocessor.Job{ ID: id, - MiningCandidate: &model.MiningCandidate{ + MiningCandidate: withIssuedCoinbase(t, &tSettings, &model.MiningCandidate{ Id: idBytes, PreviousHash: parent[:], Time: uint32(floor), - }, + }), }, time.Minute) return s, idBytes @@ -597,11 +611,11 @@ func TestSubmitMiningSolution_NTimeFloor(t *testing.T) { // own, so the offending timestamp is provably the candidate's own. s.jobStore.Set(*id, &subtreeprocessor.Job{ ID: id, - MiningCandidate: &model.MiningCandidate{ + MiningCandidate: withIssuedCoinbase(t, s.blockAssembler.settings, &model.MiningCandidate{ Id: idBytes, PreviousHash: parent[:], Time: uint32(floor - 1), - }, + }), }, time.Minute) err = submit(t, s, idBytes, nil) @@ -686,11 +700,11 @@ func TestSubmitMiningSolution_NTimeFloorAdvisoryChain(t *testing.T) { parentHash := parentHeader.Hash() s.jobStore.Set(*id, &subtreeprocessor.Job{ ID: id, - MiningCandidate: &model.MiningCandidate{ + MiningCandidate: withIssuedCoinbase(t, &tSettings, &model.MiningCandidate{ Id: idBytes, PreviousHash: parentHash[:], Time: uint32(candidateTime), - }, + }), }, time.Minute) // mining.Mine forwards candidate.Time as req.Time, so submit the same value. diff --git a/services/rpc/handlers.go b/services/rpc/handlers.go index b81935bbbd..25291f0c9e 100644 --- a/services/rpc/handlers.go +++ b/services/rpc/handlers.go @@ -1180,12 +1180,15 @@ func handleGetMiningCandidate(ctx context.Context, s *RPCServer, cmd interface{} } if c.ProvideCoinbaseTx != nil && *c.ProvideCoinbaseTx { - coinbaseTx, err := mc.CreateCoinbaseTxCandidate(s.settings, true) - if err != nil { - return nil, err + // The coinbase block assembly keeps on the candidate's job, as bitcoin-sv returns the + // candidate block's own coinbase. A solution submitted without a coinbase is mined with + // this one, so a miner that hashes it and omits it from submitminingsolution gets the block + // it hashed. A coinbase built here instead would be one block assembly never sees. + if len(mc.CoinbaseTx) == 0 { + return nil, bsvjson.NewRPCError(bsvjson.ErrRPCInternal.Code, "block assembly returned the candidate without its coinbase") } - jsonMap["coinbase"] = hex.EncodeToString(coinbaseTx.Bytes()) + jsonMap["coinbase"] = hex.EncodeToString(mc.CoinbaseTx) } if *c.Verbosity == uint32(1) { diff --git a/services/rpc/handlers_additional_test.go b/services/rpc/handlers_additional_test.go index 6c81647705..f03eb4477b 100644 --- a/services/rpc/handlers_additional_test.go +++ b/services/rpc/handlers_additional_test.go @@ -2115,6 +2115,7 @@ func TestHandleGetMiningCandidateComprehensive(t *testing.T) { NumTxs: 75, SizeWithoutCoinbase: 25000, MerkleProof: [][]byte{{0xab, 0xcd}}, + CoinbaseTx: []byte{0x01, 0x00, 0x00, 0x00, 0xc0, 0xff, 0xee}, } mockBlockAssemblyClient := &mockBlockAssemblyClient{ @@ -2154,9 +2155,10 @@ func TestHandleGetMiningCandidateComprehensive(t *testing.T) { assert.True(t, ok) assert.NotEmpty(t, coinbaseHex) - // Verify it's valid hex - _, err = hex.DecodeString(coinbaseHex) - assert.NoError(t, err) + // It is the coinbase block assembly keeps on the job, byte for byte, not one built here. + coinbase, err := hex.DecodeString(coinbaseHex) + require.NoError(t, err) + require.Equal(t, mockCandidate.CoinbaseTx, coinbase) }) t.Run("successful mining candidate with verbosity 1 (includes subtree hashes)", func(t *testing.T) { @@ -2311,7 +2313,7 @@ func TestHandleGetMiningCandidateComprehensive(t *testing.T) { assert.Nil(t, result) }) - t.Run("coinbase creation error", func(t *testing.T) { + t.Run("candidate without its coinbase is an error", func(t *testing.T) { mockCandidate := &model.MiningCandidate{ Id: []byte{0x01, 0x02, 0x03, 0x04}, PreviousHash: []byte{0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0x00, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}, @@ -2337,7 +2339,7 @@ func TestHandleGetMiningCandidateComprehensive(t *testing.T) { settings: &settings.Settings{ ChainCfgParams: &chaincfg.MainNetParams, BlockAssembly: settings.BlockAssemblySettings{ - MinerWalletPrivateKeys: []string{}, // Empty wallet addresses to trigger error + MinerWalletPrivateKeys: []string{"5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss"}, }, }, } @@ -2352,7 +2354,7 @@ func TestHandleGetMiningCandidateComprehensive(t *testing.T) { result, err := handleGetMiningCandidate(context.Background(), s, cmd, nil) assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "no wallet addresses provided") + assert.Contains(t, err.Error(), "without its coinbase") }) } From 753d31aec9478f3373f75009aa2080c30bc0250b Mon Sep 17 00:00:00 2001 From: freemans13 Date: Thu, 22 Oct 2026 11:02:39 +0100 Subject: [PATCH 2/6] docs(changelog): reference PR 2027 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77ce4f3ca9..a2b72d76fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,7 +120,7 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea output that block assembly never saw. Miners that submit their own coinbase are not affected. Upgrade block assembly no later than the RPC service: an RPC service on this build answers `getminingcandidate` with `coinbase=true` with an error while it is talking - to an older block assembly, which sends no coinbase. (#TBD) + to an older block assembly, which sends no coinbase. (#2027) - **`utxostore_updateTxMinedStatus` is removed, and it was live.** Affects only nodes that set it to `false`. Mined-tagging, the step after `AddBlock` that writes a block's ID onto From ecb0026a8b14e6fbf7b5c205273e35d11d5234b8 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Thu, 22 Oct 2026 11:51:58 +0100 Subject: [PATCH 3/6] fix(blockassembly): serve candidates without miner wallet keys (review 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 Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc --- CHANGELOG.md | 4 +- .../minersHowToInteractWithRPCServer.md | 4 +- docs/references/services/rpc_reference.md | 6 +- services/blockassembly/Server.go | 28 ++++++--- .../blockassembly/candidate_coinbase_test.go | 63 +++++++++++++++++-- services/rpc/handlers.go | 2 +- 6 files changed, 88 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd81eebaa7..1587a31575 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,7 +118,9 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea coinbase: a version 1 transaction paying the configured miner wallet addresses by P2PKH, where it used to return a separately built version 2 transaction with one pay-to-pubkey output that block assembly never saw. Miners that submit their own coinbase are not - affected. Upgrade block assembly no later than the RPC service: an RPC service on this + affected, and neither is a node without `miner_wallet_private_keys`: it still serves + candidates, with no coinbase on the job, and refuses only a coinbase-less solution and + `coinbase=true`, as it did before. Upgrade block assembly no later than the RPC service: an RPC service on this build answers `getminingcandidate` with `coinbase=true` with an error while it is talking to an older block assembly, which sends no coinbase. (#2027) diff --git a/docs/howto/miners/minersHowToInteractWithRPCServer.md b/docs/howto/miners/minersHowToInteractWithRPCServer.md index 4e09779f44..65506b41e4 100644 --- a/docs/howto/miners/minersHowToInteractWithRPCServer.md +++ b/docs/howto/miners/minersHowToInteractWithRPCServer.md @@ -38,7 +38,7 @@ The Teranode RPC server provides a JSON-RPC interface for interacting with the n - Optional object containing: - - `provideCoinbaseTx` (boolean, optional, default=false): If true, includes the coinbase transaction in the response + - `provideCoinbaseTx` (boolean, optional, default=false): If true, includes the coinbase transaction in the response. This is the coinbase block assembly keeps on the candidate's job, paying the configured `miner_wallet_private_keys` addresses, and the one a solution without its own coinbase is mined with. The call fails if block assembly could not build one, for example because no miner wallet keys are configured - `verbosity` (numeric, optional, default=0): 0 for standard response, 1 to include subtree hashes - Returns: Object containing candidate ID, previous block hash, coinbase value, and merkle proof @@ -90,7 +90,7 @@ The Teranode RPC server provides a JSON-RPC interface for interacting with the n - `id` (string, required): Mining candidate ID - `nonce` (hexadecimal string, required): Nonce value found - - `coinbase` (hexadecimal string, required): Complete coinbase transaction + - `coinbase` (hexadecimal string, optional): Complete coinbase transaction. If omitted, the block is mined with the coinbase block assembly kept on the candidate's job, the one `getminingcandidate` returns with `provideCoinbaseTx`, as in bitcoin-sv. The miner must then have hashed that coinbase. Omitting it fails if block assembly could not build one, for example because no miner wallet keys are configured - `time` (numeric, required): Block time - `version` (numeric, optional): Block version diff --git a/docs/references/services/rpc_reference.md b/docs/references/services/rpc_reference.md index 664633aefc..2e9018f5c2 100644 --- a/docs/references/services/rpc_reference.md +++ b/docs/references/services/rpc_reference.md @@ -888,7 +888,7 @@ Returns information for creating a new block. 1. `parameters` (object, optional): - - `provideCoinbaseTx` (boolean, optional, default=false): If true, includes the coinbase transaction in the response + - `provideCoinbaseTx` (boolean, optional, default=false): If true, includes the coinbase transaction in the response. This is the coinbase block assembly keeps on the candidate's job, paying the configured `miner_wallet_private_keys` addresses, and the one a solution without its own coinbase is mined with. The call fails if block assembly could not build one, for example because no miner wallet keys are configured - `verbosity` (numeric, optional, default=0): 0 for standard response, 1 to include subtree hashes **Returns:** @@ -947,12 +947,14 @@ Submits a solved block to the network. { "id": "string", // Mining candidate ID "nonce": number, // Block nonce - "coinbase": "string", // Coinbase transaction + "coinbase": "string", // Coinbase transaction (optional, see below) "time": number, // Block time "version": number // Block version } ``` + If `coinbase` is omitted, the block is mined with the coinbase block assembly kept on the candidate's job, the one `getminingcandidate` returns with `provideCoinbaseTx`, as in bitcoin-sv. The miner must then have hashed that coinbase. `version` goes into the block header only. Omitting `coinbase` fails if block assembly could not build one, for example because no miner wallet keys are configured. + **Returns:** - `boolean` - True if accepted, false if rejected diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index 2c7fd2ebd1..a8c96eebde 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -1673,9 +1673,7 @@ func (ba *BlockAssembly) GetMiningCandidate(ctx context.Context, req *blockassem id, _ := chainhash.NewHash(miningCandidate.Id) - if err = ba.storeJobWithCoinbase(id, miningCandidate, subtrees); err != nil { - return nil, errors.WrapGRPC(err) - } + ba.storeJobWithCoinbase(id, miningCandidate, subtrees) if includeSubtreeHashes { miningCandidate.SubtreeHashes = make([][]byte, len(subtrees)) @@ -1701,7 +1699,13 @@ func (ba *BlockAssembly) GetMiningCandidate(ctx context.Context, req *blockassem // so polls in the same second share one, and the empty-block path derives its id from the parent // alone. A poll for an id whose job is still held therefore reuses that job's coinbase rather than // building a new one, provided it pays the same value at the same height. -func (ba *BlockAssembly) storeJobWithCoinbase(id *chainhash.Hash, miningCandidate *model.MiningCandidate, subtrees []*subtreepkg.Subtree) error { +// +// Building the coinbase needs miner_wallet_private_keys, which a pool that submits its own coinbase +// and generatetoaddress, which pays an explicit address, have never needed. So a coinbase that +// cannot be built leaves the job without one rather than failing the candidate: only a solution +// that omits its coinbase, GetCandidateBlock and getminingcandidate with coinbase=true need it, +// and each of those refuses a job without one. +func (ba *BlockAssembly) storeJobWithCoinbase(id *chainhash.Hash, miningCandidate *model.MiningCandidate, subtrees []*subtreepkg.Subtree) { ba.jobCoinbaseMu.Lock() defer ba.jobCoinbaseMu.Unlock() @@ -1715,10 +1719,10 @@ func (ba *BlockAssembly) storeJobWithCoinbase(id *chainhash.Hash, miningCandidat if len(miningCandidate.CoinbaseTx) == 0 { coinbaseTx, err := miningCandidate.CreateCoinbaseTxCandidate(ba.blockAssembler.settings) if err != nil { - return errors.NewProcessingError("[GetMiningCandidate][%s] failed to create coinbase tx", id.String(), err) + ba.logger.Debugf("[GetMiningCandidate][%s] storing the job without a coinbase, so only a solution with its own coinbase can be mined: %v", id.String(), err) + } else { + miningCandidate.CoinbaseTx = coinbaseTx.Bytes() } - - miningCandidate.CoinbaseTx = coinbaseTx.Bytes() } ba.jobStore.Set(*id, &subtreeprocessor.Job{ @@ -1726,8 +1730,6 @@ func (ba *BlockAssembly) storeJobWithCoinbase(id *chainhash.Hash, miningCandidat Subtrees: subtrees, MiningCandidate: miningCandidate, }, jobTTL) - - return nil } // SubmitMiningSolution processes a mining solution submission. @@ -1979,6 +1981,10 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub // the one the miner hashed and every such solution failed the proof-of-work check. The // solution's version, time and nonce belong in the header, built from req below, and are not // copied into the coinbase, so it is used exactly as issued. + if len(job.MiningCandidate.CoinbaseTx) == 0 { + return nil, errors.NewProcessingError("[BlockAssembly][%s] solution has no coinbase and the job has none to mine with, because block assembly could not build one from miner_wallet_private_keys", jobID) + } + coinbaseTx, err = bt.NewTxFromBytes(job.MiningCandidate.CoinbaseTx) if err != nil { return nil, errors.NewProcessingError("[BlockAssembly][%s] job holds no usable coinbase tx", jobID, err) @@ -2323,6 +2329,10 @@ func (ba *BlockAssembly) GetCandidateBlock(ctx context.Context, req *blockassemb // The coinbase the job was issued with, so this block is the one a coinbase-less solution to the // candidate would mine. + if len(job.MiningCandidate.CoinbaseTx) == 0 { + return nil, errors.WrapGRPC(errors.NewProcessingError("[GetCandidateBlock][%s] job has no coinbase, because block assembly could not build one from miner_wallet_private_keys", candidateID)) + } + coinbaseTx, err := bt.NewTxFromBytes(job.MiningCandidate.CoinbaseTx) if err != nil { return nil, errors.WrapGRPC(errors.NewProcessingError("[GetCandidateBlock][%s] job holds no usable coinbase tx", candidateID, err)) diff --git a/services/blockassembly/candidate_coinbase_test.go b/services/blockassembly/candidate_coinbase_test.go index 15d6baff67..5735c45402 100644 --- a/services/blockassembly/candidate_coinbase_test.go +++ b/services/blockassembly/candidate_coinbase_test.go @@ -93,7 +93,7 @@ func newCandidateCoinbaseHarness(t *testing.T) *candidateCoinbaseHarness { func (h *candidateCoinbaseHarness) issue(t *testing.T) *model.MiningCandidate { t.Helper() - require.NoError(t, h.server.storeJobWithCoinbase(h.id, h.candidate, nil)) + h.server.storeJobWithCoinbase(h.id, h.candidate, nil) require.NotEmpty(t, h.candidate.CoinbaseTx, "a stored job must carry its coinbase") issued := proto.Clone(h.candidate).(*model.MiningCandidate) @@ -188,7 +188,7 @@ func TestStoreJobWithCoinbase_OncePerCandidateID(t *testing.T) { again := proto.Clone(h.candidate).(*model.MiningCandidate) again.CoinbaseTx = nil - require.NoError(t, h.server.storeJobWithCoinbase(h.id, again, nil)) + h.server.storeJobWithCoinbase(h.id, again, nil) require.Equal(t, first.CoinbaseTx, again.CoinbaseTx) require.Equal(t, first.CoinbaseTx, h.server.jobStore.Get(*h.id).Value().MiningCandidate.CoinbaseTx) }) @@ -212,7 +212,7 @@ func TestStoreJobWithCoinbase_OncePerCandidateID(t *testing.T) { poll := proto.Clone(h.candidate).(*model.MiningCandidate) poll.CoinbaseTx = nil - require.NoError(t, h.server.storeJobWithCoinbase(h.id, poll, nil)) + h.server.storeJobWithCoinbase(h.id, poll, nil) got[i] = poll.CoinbaseTx }() } @@ -233,7 +233,7 @@ func TestStoreJobWithCoinbase_OncePerCandidateID(t *testing.T) { changed.CoinbaseTx = nil changed.CoinbaseValue-- - require.NoError(t, h.server.storeJobWithCoinbase(h.id, changed, nil)) + h.server.storeJobWithCoinbase(h.id, changed, nil) require.NotEqual(t, first.CoinbaseTx, changed.CoinbaseTx) coinbase, err := bt.NewTxFromBytes(changed.CoinbaseTx) @@ -241,3 +241,58 @@ func TestStoreJobWithCoinbase_OncePerCandidateID(t *testing.T) { require.Equal(t, changed.CoinbaseValue, coinbase.TotalOutputSatoshis()) }) } + +// TestStoreJobWithCoinbase_NoMinerKeys pins that a node without miner_wallet_private_keys still +// serves candidates. A pool that submits its own coinbase, and generatetoaddress, never needed those +// keys, so a coinbase that cannot be built leaves the job without one instead of failing the +// candidate. Only the paths that need the job's coinbase refuse, and refusing does not drop the job. +func TestStoreJobWithCoinbase_NoMinerKeys(t *testing.T) { + h := newCandidateCoinbaseHarness(t) + + // The miner's own coinbase, built while the keys are still configured. + own, err := h.candidate.CreateCoinbaseTxCandidate(h.server.blockAssembler.settings) + require.NoError(t, err) + + h.server.blockAssembler.settings.BlockAssembly.MinerWalletPrivateKeys = nil + + h.server.storeJobWithCoinbase(h.id, h.candidate, nil) + require.Empty(t, h.candidate.CoinbaseTx) + + held := h.server.jobStore.Get(*h.id) + require.NotNil(t, held, "the job must be stored even though it has no coinbase") + require.Empty(t, held.Value().MiningCandidate.CoinbaseTx) + + _, err = h.server.GetCandidateBlock(t.Context(), &blockassembly_api.GetCandidateBlockRequest{Id: h.candidate.Id}) + require.ErrorContains(t, err, "miner_wallet_private_keys") + + _, err = h.server.submitMiningSolution(t.Context(), &BlockSubmissionRequest{ + SubmitMiningSolutionRequest: &blockassembly_api.SubmitMiningSolutionRequest{Id: h.candidate.Id}, + }) + require.ErrorContains(t, err, "miner_wallet_private_keys") + require.NotNil(t, h.server.jobStore.Get(*h.id), "a refused coinbase-less solution must not drop the job") + + // A solution carrying its own coinbase is mined. + hashed := proto.Clone(h.candidate).(*model.MiningCandidate) + hashed.CoinbaseTx = own.Bytes() + + version := h.candidate.Version + nonce := mine(t, hashed, version) + + resp, err := h.server.submitMiningSolution(t.Context(), &BlockSubmissionRequest{ + SubmitMiningSolutionRequest: &blockassembly_api.SubmitMiningSolutionRequest{ + Id: h.candidate.Id, + Nonce: nonce, + Version: &version, + CoinbaseTx: own.Bytes(), + }, + }) + require.NoError(t, err) + require.True(t, resp.Ok) + + best, _, err := h.store.GetBestBlockHeader(t.Context()) + require.NoError(t, err) + + block, err := h.store.GetBlock(t.Context(), best.Hash()) + require.NoError(t, err) + require.Equal(t, own.Bytes(), block.CoinbaseTx.Bytes()) +} diff --git a/services/rpc/handlers.go b/services/rpc/handlers.go index 25291f0c9e..881daded7c 100644 --- a/services/rpc/handlers.go +++ b/services/rpc/handlers.go @@ -1185,7 +1185,7 @@ func handleGetMiningCandidate(ctx context.Context, s *RPCServer, cmd interface{} // this one, so a miner that hashes it and omits it from submitminingsolution gets the block // it hashed. A coinbase built here instead would be one block assembly never sees. if len(mc.CoinbaseTx) == 0 { - return nil, bsvjson.NewRPCError(bsvjson.ErrRPCInternal.Code, "block assembly returned the candidate without its coinbase") + return nil, bsvjson.NewRPCError(bsvjson.ErrRPCInternal.Code, "block assembly returned the candidate without its coinbase: it could not build one from miner_wallet_private_keys, or it is older than this RPC service") } jsonMap["coinbase"] = hex.EncodeToString(mc.CoinbaseTx) From 738455bb1d59dccf65be7860129bc20f58e0c43d Mon Sep 17 00:00:00 2001 From: freemans13 Date: Thu, 22 Oct 2026 12:11:56 +0100 Subject: [PATCH 4/6] test(blockassembly): generatetoaddress mines without miner wallet keys 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 Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc --- .../blockassembly/candidate_coinbase_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/services/blockassembly/candidate_coinbase_test.go b/services/blockassembly/candidate_coinbase_test.go index 5735c45402..25ae42d735 100644 --- a/services/blockassembly/candidate_coinbase_test.go +++ b/services/blockassembly/candidate_coinbase_test.go @@ -296,3 +296,34 @@ func TestStoreJobWithCoinbase_NoMinerKeys(t *testing.T) { require.NoError(t, err) require.Equal(t, own.Bytes(), block.CoinbaseTx.Bytes()) } + +// TestGenerateBlocks_NoMinerKeys pins that generatetoaddress mines with miner_wallet_private_keys +// empty. It pays the given address and never needed those keys, so a candidate must still be +// served when block assembly cannot build the job's own coinbase. +func TestGenerateBlocks_NoMinerKeys(t *testing.T) { + server, _ := setupServer(t) + require.NoError(t, server.blockAssembler.Start(t.Context())) + + server.blockAssembler.settings.BlockAssembly.MinerWalletPrivateKeys = nil + + _, before, err := server.blockchainClient.GetBestBlockHeader(t.Context()) + require.NoError(t, err) + + // Testnet P2PKH address, as in TestGenerateBlock_AllowsP2PKHCoinbaseOutput. + address := "n2ZNV88uQbede7C5M5jzi6SyG4GVVr5JTt" + + _, err = server.GenerateBlocks(t.Context(), &blockassembly_api.GenerateBlocksRequest{Count: 1, Address: &address}) + require.NoError(t, err) + + best, after, err := server.blockchainClient.GetBestBlockHeader(t.Context()) + require.NoError(t, err) + require.Equal(t, before.Height+1, after.Height) + + block, err := server.blockchainClient.GetBlock(t.Context(), best.Hash()) + require.NoError(t, err) + + want, err := model.AddressToScript(address) + require.NoError(t, err) + require.Len(t, block.CoinbaseTx.Outputs, 1) + require.Equal(t, want, block.CoinbaseTx.Outputs[0].LockingScript.Bytes(), "the coinbase must pay the requested address") +} From b44c4b26d2b77c41592d83859cb751d414153e8e Mon Sep 17 00:00:00 2001 From: freemans13 Date: Fri, 23 Oct 2026 11:05:46 +0100 Subject: [PATCH 5/6] fix(blockassembly): keep a job's coinbase after an invalid solution and 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 e787b23f5 (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 Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc --- CHANGELOG.md | 7 +- services/blockassembly/Server.go | 80 ++++++++++-- .../blockassembly/candidate_coinbase_test.go | 116 +++++++++++++++++- ...it_mining_solution_coinbase_height_test.go | 13 +- services/rpc/handlers.go | 2 +- 5 files changed, 201 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b764f7f16..54e5a89781 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,7 +120,12 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea output that block assembly never saw. Miners that submit their own coinbase are not affected, and neither is a node without `miner_wallet_private_keys`: it still serves candidates, with no coinbase on the job, and refuses only a coinbase-less solution and - `coinbase=true`, as it did before. Upgrade block assembly no later than the RPC service: an RPC service on this + `coinbase=true`, as it did before. A solution that breaks a consensus rule no longer removes the job, so other + miners on the same candidate keep its coinbase and can still submit; the job goes when its + TTL lapses or a block is mined. A solution whose hash misses the target is rejected without + resetting block assembly, which it used to do. If `miner_wallet_private_keys` is set but + cannot build a coinbase, block assembly now logs a warning with the cause, at most once a + minute. Upgrade block assembly no later than the RPC service: an RPC service on this build answers `getminingcandidate` with `coinbase=true` with an error while it is talking to an older block assembly, which sends no coinbase. (#2027) diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index a8c96eebde..2f2ee92729 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -18,7 +18,9 @@ import ( "context" "fmt" "net/http" + "strings" "sync" + "sync/atomic" "time" "unsafe" @@ -115,6 +117,10 @@ type BlockAssembly struct { // one miner holding a coinbase the job no longer has. jobCoinbaseMu sync.Mutex + // lastMinerKeyWarn is when, in Unix nanoseconds, logCoinbaseBuildFailure last warned that the + // configured miner keys cannot build a coinbase. + lastMinerKeyWarn atomic.Int64 + // blockSubmissionChan handles block submission requests blockSubmissionChan chan *BlockSubmissionRequest @@ -1719,7 +1725,7 @@ func (ba *BlockAssembly) storeJobWithCoinbase(id *chainhash.Hash, miningCandidat if len(miningCandidate.CoinbaseTx) == 0 { coinbaseTx, err := miningCandidate.CreateCoinbaseTxCandidate(ba.blockAssembler.settings) if err != nil { - ba.logger.Debugf("[GetMiningCandidate][%s] storing the job without a coinbase, so only a solution with its own coinbase can be mined: %v", id.String(), err) + ba.logCoinbaseBuildFailure(id, err) } else { miningCandidate.CoinbaseTx = coinbaseTx.Bytes() } @@ -1732,6 +1738,47 @@ func (ba *BlockAssembly) storeJobWithCoinbase(id *chainhash.Hash, miningCandidat }, jobTTL) } +// minerKeyWarnInterval is the least time between two warnings that the configured +// miner_wallet_private_keys cannot build a coinbase, so a misconfigured node polled every second +// warns once a minute rather than once a poll. +const minerKeyWarnInterval = time.Minute + +// logCoinbaseBuildFailure reports a coinbase that storeJobWithCoinbase could not build. With no +// miner_wallet_private_keys configured that is expected, since a pool that submits its own coinbase +// needs none, so it stays at Debug. With keys configured it means a key is unusable, which an +// operator otherwise only sees as a generic error from getminingcandidate with coinbase=true or a +// coinbase-less submitminingsolution, so it is a warning carrying the cause. +func (ba *BlockAssembly) logCoinbaseBuildFailure(id *chainhash.Hash, err error) { + if !minerKeysConfigured(ba.blockAssembler.settings.BlockAssembly.MinerWalletPrivateKeys) { + ba.logger.Debugf("[GetMiningCandidate][%s] storing the job without a coinbase, so only a solution with its own coinbase can be mined: %v", id.String(), err) + + return + } + + now := time.Now().UnixNano() + last := ba.lastMinerKeyWarn.Load() + + if now-last < int64(minerKeyWarnInterval) || !ba.lastMinerKeyWarn.CompareAndSwap(last, now) { + ba.logger.Debugf("[GetMiningCandidate][%s] storing the job without a coinbase, miner_wallet_private_keys cannot build one: %v", id.String(), err) + + return + } + + ba.logger.Warnf("[GetMiningCandidate][%s] miner_wallet_private_keys is set but cannot build a coinbase, so candidates carry none and only solutions with their own coinbase can be mined: %v", id.String(), err) +} + +// minerKeysConfigured reports whether any miner wallet key is set. Blank entries do not count, so +// an unset variable substituted into miner_wallet_private_keys reads as no keys rather than a bad one. +func minerKeysConfigured(keys []string) bool { + for _, key := range keys { + if strings.TrimSpace(key) != "" { + return true + } + } + + return false +} + // SubmitMiningSolution processes a mining solution submission. // It validates the solution, creates a block, and adds it to the blockchain. // @@ -2111,21 +2158,38 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub CoinbaseBUMP: coinbaseBUMP, } - // check fully valid, including whether difficulty in header is low enough + // A hash that misses the target is the submitter's: the nonce, time and version are the + // solution's, and the merkle root is built from the job's subtrees and either the solution's + // coinbase or the one the job was issued with. It is checked here because block.Valid reports a + // miss as a processing error (HasMetTargetDifficulty returns one with the miss), which below + // would reset block assembly and drop the job that every other miner on this candidate is + // still hashing. As in bitcoin-sv, which answers a high hash with a rejection and leaves the + // candidate alone, the solution is refused and the job kept. + if met, _, _ := block.Header.HasMetTargetDifficulty(); !met { + ba.logger.Warnf("[BlockAssembly][%s][%s] rejected mining solution, block hash does not meet the target", jobID, block.Hash().String()) + + return nil, errors.NewProcessingError("[BlockAssembly][%s][%s] rejected mining solution, block hash does not meet the target", jobID, block.Hash().String()) + } + + // check fully valid // TODO add more checks to the Valid function, like whether the parent/child relationships are OK if ok, err := block.Valid(ctx, ba.logger, ba.subtreeStore, nil, nil, nil, nil, ba.settings, nil); !ok { // ErrBlockInvalid here means the submission breaks a consensus rule on data the miner - // chose: the proof of work, the timestamp, the block version, or the coinbase (its shape, - // BIP34 height, transaction rules or reward). Local assembly state did not cause it and a - // reset would not stop it recurring, so reject the submission and keep assembly running. + // chose: the timestamp, the block version, or the coinbase (its shape, BIP34 height, + // transaction rules or reward). The proof of work is checked above. Local assembly state did + // not cause it and a reset would not stop it recurring, so reject the submission and keep + // assembly running. // Every other failure (corrupt body, duplicate transaction, merkle mismatch, processing or // storage error) points at what this node built, so those still reset. if errors.Is(err, errors.ErrBlockInvalid) { ba.logger.Warnf("[BlockAssembly][%s][%s] rejected mining solution, block breaks a consensus rule: %v", jobID, block.Hash().String(), err) - // remove the job, the same solution would be rejected again - ba.jobStore.Delete(*storeID) - + // Keep the job. Its coinbase is the one every miner on this candidate id was handed, and + // the id outlives a single coinbase-less poll (the empty-block id is derived from the + // parent alone), so deleting it would make the next poll build a coinbase with a new + // extranonce and fail every other miner still hashing the old one, which would delete it + // again. As in bitcoin-sv, where an invalid submitminingsolution leaves the candidate + // alone, the job goes when its TTL lapses or a block is mined. return nil, errors.NewProcessingError("[BlockAssembly][%s][%s] invalid block", jobID, block.Hash().String(), err) } diff --git a/services/blockassembly/candidate_coinbase_test.go b/services/blockassembly/candidate_coinbase_test.go index 25ae42d735..54f566db48 100644 --- a/services/blockassembly/candidate_coinbase_test.go +++ b/services/blockassembly/candidate_coinbase_test.go @@ -106,6 +106,20 @@ func (h *candidateCoinbaseHarness) issue(t *testing.T) *model.MiningCandidate { func mine(t *testing.T, issued *model.MiningCandidate, version uint32) uint32 { t.Helper() + return searchNonce(t, issued, version, true) +} + +// missTarget finds a nonce whose header, built as in mine, does not meet the target. The regtest +// target passes about half of all nonces, so an arbitrary "wrong" nonce would be a coin flip. +func missTarget(t *testing.T, issued *model.MiningCandidate, version uint32) uint32 { + t.Helper() + + return searchNonce(t, issued, version, false) +} + +func searchNonce(t *testing.T, issued *model.MiningCandidate, version uint32, meetsTarget bool) uint32 { + t.Helper() + coinbase, err := bt.NewTxFromBytes(issued.CoinbaseTx) require.NoError(t, err) @@ -124,7 +138,7 @@ func mine(t *testing.T, issued *model.MiningCandidate, version uint32) uint32 { } for { - if ok, _, _ := header.HasMetTargetDifficulty(); ok { + if ok, _, _ := header.HasMetTargetDifficulty(); ok == meetsTarget { return header.Nonce } @@ -166,6 +180,106 @@ func TestSubmitMiningSolution_CoinbaseLessMinesTheIssuedCoinbase(t *testing.T) { require.Equal(t, nonce, block.Header.Nonce) } +// TestSubmitMiningSolution_InvalidSolutionKeepsTheIssuedCoinbase pins that a consensus-invalid +// solution leaves the job, and so its coinbase, in place, as bitcoin-sv leaves a candidate alone +// after an invalid submitminingsolution. Deleting the job made the next poll for the same id, which +// for an empty block is derived from the parent alone, build a coinbase with a new random +// extranonce, so every other miner still hashing the first one failed the proof-of-work check and +// deleted the job again. A share that misses the target, the commonest invalid solution, did worse: +// block.Valid reports a miss as a processing error, so it also reset block assembly. A miner on the +// original coinbase must still get its block. +func TestSubmitMiningSolution_InvalidSolutionKeepsTheIssuedCoinbase(t *testing.T) { + h := newCandidateCoinbaseHarness(t) + issued := h.issue(t) + + version := issued.Version + + // One miner's share that misses the target, coinbase-less like the miners it shares the id with. + _, err := h.server.submitMiningSolution(t.Context(), &BlockSubmissionRequest{ + SubmitMiningSolutionRequest: &blockassembly_api.SubmitMiningSolutionRequest{ + Id: issued.Id, + Nonce: missTarget(t, issued, version), + Version: &version, + }, + }) + require.ErrorContains(t, err, "block hash does not meet the target") + + // A share that misses the target is the miner's fault, so block assembly is not reset. Reset + // sends from a goroutine, so wait a short while rather than checking once. + select { + case req := <-h.server.blockAssembler.resetCh: + require.FailNow(t, "block assembly was reset", "reset request: %+v", req) + case <-time.After(250 * time.Millisecond): + } + + held := h.server.jobStore.Get(*h.id) + require.NotNil(t, held, "a consensus-invalid solution must not drop the job") + require.Equal(t, issued.CoinbaseTx, held.Value().MiningCandidate.CoinbaseTx, "the job must keep the coinbase it was issued with") + + // Another poll for the same id is handed the same coinbase. + again := proto.Clone(h.candidate).(*model.MiningCandidate) + again.CoinbaseTx = nil + h.server.storeJobWithCoinbase(h.id, again, nil) + require.Equal(t, issued.CoinbaseTx, again.CoinbaseTx) + + // A second miner, hashing the coinbase issued before the bad share, solves the block. + nonce := mine(t, issued, version) + + resp, err := h.server.submitMiningSolution(t.Context(), &BlockSubmissionRequest{ + SubmitMiningSolutionRequest: &blockassembly_api.SubmitMiningSolutionRequest{ + Id: issued.Id, + Nonce: nonce, + Version: &version, + }, + }) + require.NoError(t, err) + require.True(t, resp.Ok) + + best, _, err := h.store.GetBestBlockHeader(t.Context()) + require.NoError(t, err) + require.Equal(t, h.tip.Hash(), best.HashPrevBlock) + + block, err := h.store.GetBlock(t.Context(), best.Hash()) + require.NoError(t, err) + require.Equal(t, issued.CoinbaseTx, block.CoinbaseTx.Bytes(), "the block must carry the coinbase issued before the invalid share") + require.Equal(t, nonce, block.Header.Nonce) +} + +// TestStoreJobWithCoinbase_UnusableMinerKeysWarn pins that a configured miner key that cannot build a +// coinbase is reported at Warn with its cause, so it is not mistaken for a node with no keys, which +// stays quiet. A node polled every second warns once per minerKeyWarnInterval, not once a poll. +func TestStoreJobWithCoinbase_UnusableMinerKeysWarn(t *testing.T) { + t.Run("a key that is not a WIF warns once with the cause", func(t *testing.T) { + h := newCandidateCoinbaseHarness(t) + logger := &warnCapturingLogger{} + h.server.logger = logger + h.server.blockAssembler.settings.BlockAssembly.MinerWalletPrivateKeys = []string{"not-a-wif"} + + for range 3 { + poll := proto.Clone(h.candidate).(*model.MiningCandidate) + h.server.storeJobWithCoinbase(h.id, poll, nil) + require.Empty(t, poll.CoinbaseTx) + } + + require.Len(t, logger.warns, 1, "repeated polls within the interval must warn once") + require.Contains(t, logger.warns[0], "miner_wallet_private_keys is set but cannot build a coinbase") + require.Contains(t, logger.warns[0], "can't decode coinbase priv key", "the warning must carry the cause") + }) + + t.Run("no keys, or only blank ones, stays quiet", func(t *testing.T) { + for _, keys := range [][]string{nil, {""}, {" "}} { + h := newCandidateCoinbaseHarness(t) + logger := &warnCapturingLogger{} + h.server.logger = logger + h.server.blockAssembler.settings.BlockAssembly.MinerWalletPrivateKeys = keys + + h.server.storeJobWithCoinbase(h.id, h.candidate, nil) + require.Empty(t, h.candidate.CoinbaseTx) + require.Empty(t, logger.warns, "keys %q must not warn", keys) + } + }) +} + // TestGetCandidateBlock_CarriesTheIssuedCoinbase pins that the candidate block served for a job is // built from the same coinbase the job was issued with, not one built on the spot. func TestGetCandidateBlock_CarriesTheIssuedCoinbase(t *testing.T) { diff --git a/services/blockassembly/submit_mining_solution_coinbase_height_test.go b/services/blockassembly/submit_mining_solution_coinbase_height_test.go index dfc745c8cd..2a51a62a68 100644 --- a/services/blockassembly/submit_mining_solution_coinbase_height_test.go +++ b/services/blockassembly/submit_mining_solution_coinbase_height_test.go @@ -333,17 +333,18 @@ func TestSubmitMiningSolution_CoinbaseRulesUseCandidateHeight(t *testing.T) { require.Contains(t, err.Error(), "block height in coinbase tx") h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) - // The wrong height is the pool's, not assembly's: the job is dropped but assembly is not - // reset, since a reset would not stop the same pool doing it again. - require.Nil(t, h.server.jobStore.Get(h.id), "the rejected job must be deleted") + // The wrong height is the pool's, not assembly's: assembly is not reset, since a reset would + // not stop the same pool doing it again, and the job is kept for the other miners on it. + require.NotNil(t, h.server.jobStore.Get(h.id), "a consensus-invalid solution must not drop the job") h.requireNoReset(t) }) } // TestSubmitMiningSolution_InvalidBlockResetOnlyForLocalFaults pins what submitMiningSolution does // with a block that fails Block.Valid. A consensus verdict on data the submitter chose -// (ErrBlockInvalid) says nothing about local assembly state, so it drops the job without a reset. -// Any other failure points at the block this node assembled, so it still resets. +// (ErrBlockInvalid) says nothing about local assembly state, so it keeps the job, which other miners +// may still be hashing, and does not reset. Any other failure points at the block this node +// assembled, so it still resets and drops the job. func TestSubmitMiningSolution_InvalidBlockResetOnlyForLocalFaults(t *testing.T) { t.Run("a coinbase that overpays is rejected without a reset", func(t *testing.T) { params := chaincfg.RegressionNetParams @@ -358,7 +359,7 @@ func TestSubmitMiningSolution_InvalidBlockResetOnlyForLocalFaults(t *testing.T) require.Nil(t, resp) require.Contains(t, err.Error(), "is greater than the fees + block subsidy") h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) - require.Nil(t, h.server.jobStore.Get(h.id), "the rejected job must be deleted") + require.NotNil(t, h.server.jobStore.Get(h.id), "a consensus-invalid solution must not drop the job") h.requireNoReset(t) }) diff --git a/services/rpc/handlers.go b/services/rpc/handlers.go index 881daded7c..564027730f 100644 --- a/services/rpc/handlers.go +++ b/services/rpc/handlers.go @@ -1185,7 +1185,7 @@ func handleGetMiningCandidate(ctx context.Context, s *RPCServer, cmd interface{} // this one, so a miner that hashes it and omits it from submitminingsolution gets the block // it hashed. A coinbase built here instead would be one block assembly never sees. if len(mc.CoinbaseTx) == 0 { - return nil, bsvjson.NewRPCError(bsvjson.ErrRPCInternal.Code, "block assembly returned the candidate without its coinbase: it could not build one from miner_wallet_private_keys, or it is older than this RPC service") + return nil, bsvjson.NewRPCError(bsvjson.ErrRPCInternal.Code, "block assembly returned the candidate without its coinbase: it could not build one from miner_wallet_private_keys, or it is an older or other implementation that does not set coinbase_tx") } jsonMap["coinbase"] = hex.EncodeToString(mc.CoinbaseTx) From 27f4704fe39d0933f45547460c388dd8c48af6f3 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Fri, 23 Oct 2026 11:28:51 +0100 Subject: [PATCH 6/6] test(blockassembly): keep requirePastFloorGuard's doc comment on it 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 Claude-Session: https://claude.ai/code/session_014XzzaR3W5oM3wemXLEeNFc --- .../submit_mining_solution_test.go | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/services/blockassembly/submit_mining_solution_test.go b/services/blockassembly/submit_mining_solution_test.go index 9c3105e575..7785754e98 100644 --- a/services/blockassembly/submit_mining_solution_test.go +++ b/services/blockassembly/submit_mining_solution_test.go @@ -471,17 +471,6 @@ func TestSubmitMiningSolution_NoCurrentBlock(t *testing.T) { require.Contains(t, err.Error(), "no current best block") } -// requirePastFloorGuard asserts that a submission got past the median-time-past -// guard, and does so durably. -// -// The accept direction cannot assert success: these fixtures carry no real -// subtrees or nBits, so the submission always dies further down. Asserting only -// "the error does not mention the floor" would pass just as happily if a future -// fixture change made it die *above* the guard, or stop erroring at all — the -// assertion would be vacuous and nothing would say so. Pinning the specific -// downstream failure keeps it honest: if the fixture stops dying exactly there, -// this fails loudly and whoever changed it re-checks that the guard is still on -// the path being exercised. // withIssuedCoinbase gives a hand-built candidate the coinbase GetMiningCandidate would have stored // on its job, which a solution without its own coinbase is mined with. func withIssuedCoinbase(t *testing.T, tSettings *settings.Settings, candidate *model.MiningCandidate) *model.MiningCandidate { @@ -495,6 +484,17 @@ func withIssuedCoinbase(t *testing.T, tSettings *settings.Settings, candidate *m return candidate } +// requirePastFloorGuard asserts that a submission got past the median-time-past +// guard, and does so durably. +// +// The accept direction cannot assert success: these fixtures carry no real +// subtrees or nBits, so the submission always dies further down. Asserting only +// "the error does not mention the floor" would pass just as happily if a future +// fixture change made it die *above* the guard, or stop erroring at all — the +// assertion would be vacuous and nothing would say so. Pinning the specific +// downstream failure keeps it honest: if the fixture stops dying exactly there, +// this fails loudly and whoever changed it re-checks that the guard is still on +// the path being exercised. func requirePastFloorGuard(t *testing.T, err error) { t.Helper()