From 61f7b7b044acd531fb826db43f7dcebe6d9126a1 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 21 Oct 2026 11:42:30 +0100 Subject: [PATCH 1/5] fix(blockvalidation): an in-flight tagging run no longer overwrites an invalidation's mined_set reset setTxMinedStatus reads the block's invalid flag once, tags or untags the block's transactions, then sets mined_set = true. That write was an unconditional UPDATE. If InvalidateBlock or RevalidateBlock ran in between, they had cleared mined_set so the block would be processed again; the overwrite made the setMined worker's MinedSet guard and the mined-not-set sweep both skip it for good. The result was an invalid block with its transactions still tagged, or a revalidated block with none. SetBlockMinedSet now takes the invalid flag the run started from and only writes while the row still has it. A mismatch returns ErrStateError, and setTxMinedStatus re-queues the block and returns nil, so the refusal does not charge the retry counter. The flag travels in a new expectInvalid field on SetBlockMinedSetRequest. The GAP-BV-058 proof test is now an untagged regression test that also asserts the re-queue and the nil return. New store tests cover both directions of the race on sqlite and on postgres, and a client test checks ErrStateError survives the gRPC round trip. Refs bitcoin-sv/teranode#4989 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013BLSCaXeSCCbSqjX4s5KBe --- .../blockassembly_system_test.go | 2 +- services/blockchain/Client.go | 9 +- services/blockchain/Interface.go | 6 +- services/blockchain/LocalClient.go | 4 +- services/blockchain/LocalClient_test.go | 2 +- services/blockchain/Server.go | 2 +- .../blockchain_api/blockchain_api.pb.go | 21 +- .../blockchain_api/blockchain_api.proto | 3 + services/blockchain/client_test.go | 21 +- services/blockchain/mock.go | 8 +- services/blockchain/server_test.go | 4 +- services/blockpersister/Server_test.go | 2 +- services/blockvalidation/BlockValidation.go | 18 +- .../blockvalidation/BlockValidation_test.go | 4 +- .../InvalidBlocksKafkaProducer_test.go | 4 +- .../cached_headers_median_window_test.go | 2 +- .../checkpoint_difficulty_reconsider_test.go | 2 +- .../corrupt_escalation_test.go | 4 +- services/blockvalidation/gap_bv058_test.go | 247 ++++++++++++++++++ .../header_context_requeue_test.go | 2 +- .../header_run_walk_fallback_test.go | 2 +- .../invalid_block_reconsider_test.go | 2 +- .../legacy/netsync/block_park_policy_test.go | 2 +- .../netsync/legacy_validation_stack_test.go | 2 +- stores/blockchain/Interface.go | 8 +- stores/blockchain/mock.go | 2 +- stores/blockchain/sql/RevalidateBlock_test.go | 2 +- stores/blockchain/sql/SetBlockMinedSet.go | 34 ++- .../blockchain/sql/SetBlockMinedSet_test.go | 175 +++++++++++++ 29 files changed, 546 insertions(+), 50 deletions(-) create mode 100644 services/blockvalidation/gap_bv058_test.go create mode 100644 stores/blockchain/sql/SetBlockMinedSet_test.go diff --git a/services/blockassembly/blockassembly_system_test.go b/services/blockassembly/blockassembly_system_test.go index 0190786b24..32e95ef04c 100644 --- a/services/blockassembly/blockassembly_system_test.go +++ b/services/blockassembly/blockassembly_system_test.go @@ -1181,7 +1181,7 @@ func TestHandleReorgWithInvalidBlock_Integration(t *testing.T) { // so call SetBlockMinedSet directly — otherwise BA's reset() blocks in // waitForBlockMinedSet until its retry budget exhausts. for i := range invalidatedHashes { - require.NoError(t, ba.blockchainClient.SetBlockMinedSet(ctx, &invalidatedHashes[i]), + require.NoError(t, ba.blockchainClient.SetBlockMinedSet(ctx, &invalidatedHashes[i], true), "failed to re-set mined_set on invalidated block %s", invalidatedHashes[i].String()) } diff --git a/services/blockchain/Client.go b/services/blockchain/Client.go index 247c3630c8..c5c21feea9 100644 --- a/services/blockchain/Client.go +++ b/services/blockchain/Client.go @@ -1683,12 +1683,15 @@ func (c *Client) GetBlockIsMined(ctx context.Context, blockHash *chainhash.Hash) // Parameters: // - ctx: Context for the operation with timeout and cancellation support // - blockHash: Hash of the block to mark as mined +// - expectInvalid: The block's invalid flag when the caller's tagging run started // // Returns: -// - error: Any error encountered during the mining status update operation -func (c *Client) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { +// - error: errors.ErrStateError if the block's invalid flag has changed, or any +// other error encountered during the mining status update operation +func (c *Client) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { _, err := c.client.SetBlockMinedSet(ctx, &blockchain_api.SetBlockMinedSetRequest{ - BlockHash: blockHash[:], + BlockHash: blockHash[:], + ExpectInvalid: expectInvalid, }) if err != nil { return errors.UnwrapGRPC(err) diff --git a/services/blockchain/Interface.go b/services/blockchain/Interface.go index 71a05398ae..0dca737f29 100644 --- a/services/blockchain/Interface.go +++ b/services/blockchain/Interface.go @@ -602,10 +602,14 @@ type ClientI interface { // Parameters: // - ctx: Context for the operation with timeout and cancellation support // - blockHash: Hash of the block to mark as mined + // - expectInvalid: The block's invalid flag when the caller's tagging run started. + // The write is refused if the flag has changed since, because InvalidateBlock or + // RevalidateBlock cleared mined_set so the block is processed again. // // Returns: + // - errors.ErrStateError if the block's invalid flag no longer equals expectInvalid // - Error if the operation fails, nil on success - SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error + SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error // ClearBlockMinedSet resets the mined_set flag to false for a block. // diff --git a/services/blockchain/LocalClient.go b/services/blockchain/LocalClient.go index 663ed27d86..28e6a97cbd 100644 --- a/services/blockchain/LocalClient.go +++ b/services/blockchain/LocalClient.go @@ -388,8 +388,8 @@ func (c *LocalClient) GetBlockIsMined(ctx context.Context, blockHash *chainhash. return c.store.GetBlockIsMined(ctx, blockHash) } -func (c *LocalClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { - return c.store.SetBlockMinedSet(ctx, blockHash) +func (c *LocalClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { + return c.store.SetBlockMinedSet(ctx, blockHash, expectInvalid) } func (c *LocalClient) ClearBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { diff --git a/services/blockchain/LocalClient_test.go b/services/blockchain/LocalClient_test.go index 5fd26e2686..684da3521c 100644 --- a/services/blockchain/LocalClient_test.go +++ b/services/blockchain/LocalClient_test.go @@ -370,7 +370,7 @@ func TestLocalClient_NilStorePanics(t *testing.T) { { name: "SetBlockMinedSet", fn: func() { - _ = client.SetBlockMinedSet(ctx, &blockHash) + _ = client.SetBlockMinedSet(ctx, &blockHash, false) }, }, { diff --git a/services/blockchain/Server.go b/services/blockchain/Server.go index 4cfec98c23..67a596251a 100644 --- a/services/blockchain/Server.go +++ b/services/blockchain/Server.go @@ -2978,7 +2978,7 @@ func (b *Blockchain) SetBlockMinedSet(ctx context.Context, req *blockchain_api.S return nil, errors.WrapGRPC(errors.NewInvalidArgumentError("[Blockchain][SetBlockMinedSet] hash must be %d bytes, got %d", chainhash.HashSize, len(req.BlockHash))) } - err = b.store.SetBlockMinedSet(ctx, blockHash) + err = b.store.SetBlockMinedSet(ctx, blockHash, req.ExpectInvalid) if err != nil { return nil, errors.WrapGRPC(err) } diff --git a/services/blockchain/blockchain_api/blockchain_api.pb.go b/services/blockchain/blockchain_api/blockchain_api.pb.go index 8146b68646..0bb5d9af35 100644 --- a/services/blockchain/blockchain_api/blockchain_api.pb.go +++ b/services/blockchain/blockchain_api/blockchain_api.pb.go @@ -1,7 +1,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: // protoc-gen-go v1.36.11 -// protoc v6.33.2 +// protoc v7.36.2 // source: services/blockchain/blockchain_api/blockchain_api.proto // Package blockchain_api defines the gRPC service interface for blockchain operations. @@ -3505,8 +3505,11 @@ func (x *GetNextWorkRequiredResponse) GetBits() []byte { // SetBlockMinedSetRequest marks a block as mined. type SetBlockMinedSetRequest struct { - state protoimpl.MessageState `protogen:"open.v1"` - BlockHash []byte `protobuf:"bytes,1,opt,name=blockHash,proto3" json:"blockHash,omitempty"` // Hash of the mined block + state protoimpl.MessageState `protogen:"open.v1"` + BlockHash []byte `protobuf:"bytes,1,opt,name=blockHash,proto3" json:"blockHash,omitempty"` // Hash of the mined block + // The block's invalid flag when the caller's tagging run started; the write is + // refused with ERR_STATE_ERROR if the flag has changed since. + ExpectInvalid bool `protobuf:"varint,2,opt,name=expectInvalid,proto3" json:"expectInvalid,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -3548,6 +3551,13 @@ func (x *SetBlockMinedSetRequest) GetBlockHash() []byte { return nil } +func (x *SetBlockMinedSetRequest) GetExpectInvalid() bool { + if x != nil { + return x.ExpectInvalid + } + return false +} + type ClearBlockMinedSetRequest struct { state protoimpl.MessageState `protogen:"open.v1"` BlockHash []byte `protobuf:"bytes,1,opt,name=blockHash,proto3" json:"blockHash,omitempty"` // Hash of the block to reset @@ -7609,9 +7619,10 @@ const file_services_blockchain_blockchain_api_blockchain_api_proto_rawDesc = "" "\x11previousBlockHash\x18\x01 \x01(\fR\x11previousBlockHash\x12*\n" + "\x10currentBlockTime\x18\x02 \x01(\x03R\x10currentBlockTime\"1\n" + "\x1bGetNextWorkRequiredResponse\x12\x12\n" + - "\x04bits\x18\x01 \x01(\fR\x04bits\"7\n" + + "\x04bits\x18\x01 \x01(\fR\x04bits\"]\n" + "\x17SetBlockMinedSetRequest\x12\x1c\n" + - "\tblockHash\x18\x01 \x01(\fR\tblockHash\"9\n" + + "\tblockHash\x18\x01 \x01(\fR\tblockHash\x12$\n" + + "\rexpectInvalid\x18\x02 \x01(\bR\rexpectInvalid\"9\n" + "\x19ClearBlockMinedSetRequest\x12\x1c\n" + "\tblockHash\x18\x01 \x01(\fR\tblockHash\">\n" + "\x1cGetBlocksMinedNotSetResponse\x12\x1e\n" + diff --git a/services/blockchain/blockchain_api/blockchain_api.proto b/services/blockchain/blockchain_api/blockchain_api.proto index 6520cc23d9..41d26f2503 100644 --- a/services/blockchain/blockchain_api/blockchain_api.proto +++ b/services/blockchain/blockchain_api/blockchain_api.proto @@ -618,6 +618,9 @@ message GetNextWorkRequiredResponse { // SetBlockMinedSetRequest marks a block as mined. message SetBlockMinedSetRequest { bytes blockHash = 1; // Hash of the mined block + // The block's invalid flag when the caller's tagging run started; the write is + // refused with ERR_STATE_ERROR if the flag has changed since. + bool expectInvalid = 2; } message ClearBlockMinedSetRequest { diff --git a/services/blockchain/client_test.go b/services/blockchain/client_test.go index d785187fb8..00f4ffdd87 100644 --- a/services/blockchain/client_test.go +++ b/services/blockchain/client_test.go @@ -4365,7 +4365,7 @@ func TestClientSetBlockMinedSet(t *testing.T) { settings: tSettings, } - err := c.SetBlockMinedSet(ctx, &blockHash) + err := c.SetBlockMinedSet(ctx, &blockHash, false) require.NoError(t, err) }) @@ -4379,10 +4379,27 @@ func TestClientSetBlockMinedSet(t *testing.T) { settings: tSettings, } - err := c.SetBlockMinedSet(ctx, &blockHash) + err := c.SetBlockMinedSet(ctx, &blockHash, false) require.Error(t, err) assert.Contains(t, err.Error(), "grpc error") }) + + t.Run("refused write keeps ErrStateError across gRPC", func(t *testing.T) { + // setTxMinedStatus re-queues the block only if errors.Is(err, ErrStateError) + // still holds after the server's WrapGRPC and the client's UnwrapGRPC + c := &Client{ + client: &mockBlockClient{ + err: errors.WrapGRPC(errors.NewStateError("invalid flag changed")), + }, + logger: logger, + settings: tSettings, + } + + err := c.SetBlockMinedSet(ctx, &blockHash, true) + require.Error(t, err) + assert.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) + assert.True(t, c.client.(*mockBlockClient).lastSetBlockMinedSetReq.ExpectInvalid, "expectInvalid must reach the request") + }) } // TestClientSetBlockProcessedAt tests the SetBlockProcessedAt method diff --git a/services/blockchain/mock.go b/services/blockchain/mock.go index 75e44580a3..fa9a9a5f4e 100644 --- a/services/blockchain/mock.go +++ b/services/blockchain/mock.go @@ -379,8 +379,8 @@ func (m *Mock) SetState(ctx context.Context, key string, data []byte) error { } // SetBlockMinedSet mocks the SetBlockMinedSet method -func (m *Mock) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { - args := m.Called(ctx, blockHash) +func (m *Mock) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { + args := m.Called(ctx, blockHash, expectInvalid) return args.Error(0) } @@ -770,8 +770,8 @@ func (m *mockStoreGetSetBlockIsMined) GetBlockIsMined(ctx context.Context, hash return args.Bool(0), nil } -func (m *mockStoreGetSetBlockIsMined) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash) error { - args := m.Called(ctx, hash) +func (m *mockStoreGetSetBlockIsMined) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash, expectInvalid bool) error { + args := m.Called(ctx, hash, expectInvalid) return args.Error(0) } diff --git a/services/blockchain/server_test.go b/services/blockchain/server_test.go index aae1b9f6c8..2e68c37ea2 100644 --- a/services/blockchain/server_test.go +++ b/services/blockchain/server_test.go @@ -2543,7 +2543,7 @@ func TestSetBlockMinedSet(t *testing.T) { t.Run("error - store returns error", func(t *testing.T) { hash := chainhash.DoubleHashH([]byte("error-case")) mockStore := &mockStoreGetSetBlockIsMined{} - mockStore.On("SetBlockMinedSet", mock.Anything, &hash). + mockStore.On("SetBlockMinedSet", mock.Anything, &hash, false). Return(errors.NewProcessingError("forced error")) server, _ := New(ctx, logger, tSettings, mockStore, nil) @@ -2561,7 +2561,7 @@ func TestSetBlockMinedSet(t *testing.T) { t.Run("success - block marked as mined", func(t *testing.T) { hash := chainhash.DoubleHashH([]byte("success-case")) mockStore := &mockStoreGetSetBlockIsMined{} - mockStore.On("SetBlockMinedSet", mock.Anything, &hash). + mockStore.On("SetBlockMinedSet", mock.Anything, &hash, false). Return(nil) server, _ := New(ctx, logger, tSettings, mockStore, nil) diff --git a/services/blockpersister/Server_test.go b/services/blockpersister/Server_test.go index eb2eaf54c9..1e4510d53b 100644 --- a/services/blockpersister/Server_test.go +++ b/services/blockpersister/Server_test.go @@ -759,7 +759,7 @@ func (m *MockBlockchainClient) GetMedianTimePastRange(_ context.Context, fromHei func (m *MockBlockchainClient) SetState(ctx context.Context, key string, data []byte) error { return nil } -func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { +func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { return nil } func (m *MockBlockchainClient) ClearBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { diff --git a/services/blockvalidation/BlockValidation.go b/services/blockvalidation/BlockValidation.go index 088d668e06..dcd85e5fee 100644 --- a/services/blockvalidation/BlockValidation.go +++ b/services/blockvalidation/BlockValidation.go @@ -1568,8 +1568,22 @@ func (u *BlockValidation) setTxMinedStatus(ctx context.Context, blockHash *chain u.logger.Warnf("[setTxMined][%s] failed closing subtrees after setting mined: %v", block.Hash().String(), releaseErr) } - // update block mined_set to true - if err = u.blockchainClient.SetBlockMinedSet(ctx, blockHash); err != nil { + // update block mined_set to true, but only if the block's invalid flag is still the one + // this run started from. InvalidateBlock and RevalidateBlock flip that flag and clear + // mined_set so the block is processed again; overwriting mined_set here would make the + // worker's MinedSet guard and the mined-not-set sweep both skip the block for good, + // leaving an invalid block tagged or a revalidated block untagged. + startedInvalid := len(unsetMined) > 0 && unsetMined[0] + if err = u.blockchainClient.SetBlockMinedSet(ctx, blockHash, startedInvalid); err != nil { + if errors.Is(err, errors.ErrStateError) { + // Not a failure of this run: re-queue so the block is processed again under its + // new invalid flag, and return nil so the retry counter is not charged. + u.logger.Warnf("[setTxMined][%s] block invalid flag changed during the run, re-queueing: %v", blockHash.String(), err) + u.enqueueSetMined(blockHash) + + return nil + } + return errors.NewServiceError("[setTxMined][%s] failed to set block mined", block.Hash().String(), err) } diff --git a/services/blockvalidation/BlockValidation_test.go b/services/blockvalidation/BlockValidation_test.go index ab85a9a9bb..4e55b27b82 100644 --- a/services/blockvalidation/BlockValidation_test.go +++ b/services/blockvalidation/BlockValidation_test.go @@ -241,13 +241,13 @@ func (t *trackingBlockchainClient) Subscribe(ctx context.Context, tag string) (c return t.ClientI.Subscribe(ctx, tag) } -func (t *trackingBlockchainClient) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash) error { +func (t *trackingBlockchainClient) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash, expectInvalid bool) error { if t.setMinedCalled != nil { t.setMinedOnce.Do(func() { close(t.setMinedCalled) }) } - return t.ClientI.SetBlockMinedSet(ctx, hash) + return t.ClientI.SetBlockMinedSet(ctx, hash, expectInvalid) } func (m *MockSubtreeValidationClient) Health(ctx context.Context, checkLiveness bool) (int, string, error) { diff --git a/services/blockvalidation/InvalidBlocksKafkaProducer_test.go b/services/blockvalidation/InvalidBlocksKafkaProducer_test.go index a38ac3cb19..554a0910f0 100644 --- a/services/blockvalidation/InvalidBlocksKafkaProducer_test.go +++ b/services/blockvalidation/InvalidBlocksKafkaProducer_test.go @@ -400,8 +400,8 @@ func (m *MockBlockchainClient) SendNotification(ctx context.Context, notificatio } // SetBlockMinedSet implements the blockchain.ClientI interface -func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { - args := m.Called(ctx, blockHash) +func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { + args := m.Called(ctx, blockHash, expectInvalid) return args.Error(0) } diff --git a/services/blockvalidation/cached_headers_median_window_test.go b/services/blockvalidation/cached_headers_median_window_test.go index 94be649f9e..d494b882f6 100644 --- a/services/blockvalidation/cached_headers_median_window_test.go +++ b/services/blockvalidation/cached_headers_median_window_test.go @@ -323,7 +323,7 @@ func runHeaderContextRetryReachesAddBlock(t *testing.T, disableOptimisticMining mockBlockchain.On("SetBlockSubtreesSet", mock.Anything, mock.Anything).Return(nil).Maybe() mockBlockchain.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything). Return([]uint32{1, 0}, nil).Maybe() - mockBlockchain.On("SetBlockMinedSet", mock.Anything, mock.Anything).Return(nil).Maybe() + mockBlockchain.On("SetBlockMinedSet", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() mockBlockchain.On("InvalidateBlock", mock.Anything, mock.Anything).Return([]chainhash.Hash{}, nil).Maybe() mockBlockchain.On("GetBlockHeadersFromHeight", mock.Anything, mock.Anything, mock.Anything). Return(goodRun, goodMeta, nil).Maybe() diff --git a/services/blockvalidation/checkpoint_difficulty_reconsider_test.go b/services/blockvalidation/checkpoint_difficulty_reconsider_test.go index eb12a8d7a8..8379ed9235 100644 --- a/services/blockvalidation/checkpoint_difficulty_reconsider_test.go +++ b/services/blockvalidation/checkpoint_difficulty_reconsider_test.go @@ -113,7 +113,7 @@ func TestReconsider_BelowCheckpoint_RunsExpectedNBitsAndSucceeds(t *testing.T) { // Reconsider the ancestor. RevalidateBlock clears mined_set, so re-stamp it the way the // setMined worker would; the descendant's validation waits on the parent's mined state. require.NoError(t, client.RevalidateBlock(ctx, parent.Hash())) - require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash())) + require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash(), false)) // The difficulty inputs must still be reachable after the invalidation, which is the whole // claim: the calculator walks parent_id and does not filter invalid rows. diff --git a/services/blockvalidation/corrupt_escalation_test.go b/services/blockvalidation/corrupt_escalation_test.go index 6b90758271..87f1f0d4af 100644 --- a/services/blockvalidation/corrupt_escalation_test.go +++ b/services/blockvalidation/corrupt_escalation_test.go @@ -261,7 +261,7 @@ func TestOptimisticCorrupt_ReValidationConvergesOnInvalidation(t *testing.T) { // waitForPreviousBlocksToBeProcessed gates on the parent being mined, and the store writes // genesis unmined; without this the optimistic AddBlock is never reached. - require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash)) + require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, false)) blockchainClient := &invalidateFailsFirstNClient{ClientI: realClient, failFirst: 1} @@ -867,7 +867,7 @@ func TestOptimisticCorrupt_StrikeGatedOnRevalidation(t *testing.T) { // waitForPreviousBlocksToBeProcessed gates on the parent being mined, and the store writes // genesis unmined; without this the optimistic AddBlock is never reached. - require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash)) + require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, false)) // Height 1, so the model height, the store-derived height and the BIP34 coinbase-encoded // height all agree (StoreBlock re-derives the height from the parent chain). diff --git a/services/blockvalidation/gap_bv058_test.go b/services/blockvalidation/gap_bv058_test.go new file mode 100644 index 0000000000..c256241486 --- /dev/null +++ b/services/blockvalidation/gap_bv058_test.go @@ -0,0 +1,247 @@ +package blockvalidation + +import ( + "context" + "net/url" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/bsv-blockchain/go-bt/v2" + "github.com/bsv-blockchain/go-bt/v2/bscript" + "github.com/bsv-blockchain/go-bt/v2/chainhash" + "github.com/bsv-blockchain/go-bt/v2/unlocker" + bec "github.com/bsv-blockchain/go-sdk/primitives/ec" + subtreepkg "github.com/bsv-blockchain/go-subtree" + txmap "github.com/bsv-blockchain/go-tx-map" + "github.com/bsv-blockchain/teranode/errors" + "github.com/bsv-blockchain/teranode/model" + "github.com/bsv-blockchain/teranode/pkg/fileformat" + "github.com/bsv-blockchain/teranode/services/blockchain" + "github.com/bsv-blockchain/teranode/services/subtreevalidation" + blobmemory "github.com/bsv-blockchain/teranode/stores/blob/memory" + blockchain_store "github.com/bsv-blockchain/teranode/stores/blockchain" + utxostore "github.com/bsv-blockchain/teranode/stores/utxo" + "github.com/bsv-blockchain/teranode/stores/utxo/sql" + "github.com/bsv-blockchain/teranode/ulogger" + "github.com/bsv-blockchain/teranode/util/expiringmap" + "github.com/bsv-blockchain/teranode/util/test" + "github.com/ordishs/gocore" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// gap058BlockingStore wraps a real UTXO store and parks the first non-revert SetMinedMulti call +// (a set-mined tagging run) until released, so the test can invalidate the block mid-run. +type gap058BlockingStore struct { + utxostore.Store + + armed atomic.Bool + once sync.Once + entered chan struct{} + release chan struct{} +} + +func (s *gap058BlockingStore) SetMinedMulti(ctx context.Context, hashes []*chainhash.Hash, info utxostore.MinedBlockInfo) (map[chainhash.Hash][]uint32, error) { + if s.armed.Load() && !info.UnsetMined { + s.once.Do(func() { close(s.entered) }) + + select { + case <-s.release: + case <-time.After(20 * time.Second): + return nil, errors.NewProcessingError("gap058: test never released the blocked SetMinedMulti") + } + } + + return s.Store.SetMinedMulti(ctx, hashes, info) +} + +// TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation is the regression test for GAP-BV-058 +// (BV-COMMIT-005, BV-COMMIT-011, BV-REVALIDATE-003). +// +// A set-mined tagging run that is in flight when the block is invalidated must not leave the block +// invalid with mined_set=true and its transactions still tagged. InvalidateBlock clears mined_set so +// the block is processed again; an unconditional SetBlockMinedSet at the end of the run used to +// overwrite that, after which the setMined worker's MinedSet guard and the mined-not-set sweep both +// skipped the block for good. SetBlockMinedSet is now conditional on the invalid flag the run started +// from, and setTxMinedStatus re-queues the block when the write is refused. The race is made +// deterministic with a UTXO-store decorator that parks the tagging run inside SetMinedMulti while +// InvalidateBlock runs on the real sqlitememory store. +func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { + initPrometheusMetrics() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + logger := ulogger.TestLogger{} + tSettings := test.CreateBaseTestSettings(t) + + blockChainStore, err := blockchain_store.NewStore(logger, &url.URL{Scheme: "sqlitememory"}, tSettings) + require.NoError(t, err) + + blockchainClient, err := blockchain.NewLocalClient(logger, tSettings, blockChainStore, nil, nil) + require.NoError(t, err) + + utxoStoreURL, err := url.Parse("sqlitememory:///gap058") + require.NoError(t, err) + + realUtxoStore, err := sql.New(ctx, logger, tSettings, utxoStoreURL) + require.NoError(t, err) + + subtreeStore := blobmemory.New() + txStore := blobmemory.New() + + subtreeValidationClient := &subtreevalidation.MockSubtreeValidation{} + subtreeValidationClient.On("CheckBlockSubtrees", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + + privateKey, err := bec.NewPrivateKey() + require.NoError(t, err) + address, err := bscript.NewAddressFromPublicKey(privateKey.PubKey(), true) + require.NoError(t, err) + + fundingTx := bt.NewTx() + require.NoError(t, fundingTx.From("0000000000000000000000000000000000000000000000000000000000000000", 0xffffffff, "", 0)) + fundingTx.Inputs[0].UnlockingScript = bscript.NewFromBytes([]byte{0x03, 0x64, 0x00, 0x00, 0x00, '/', 'F', 'u', 'n', 'd'}) + require.NoError(t, fundingTx.AddP2PKHOutputFromAddress(address.AddressString, 50*100000000)) + + txT := bt.NewTx() + require.NoError(t, txT.FromUTXOs(&bt.UTXO{ + TxIDHash: fundingTx.TxIDChainHash(), + Vout: 0, + LockingScript: fundingTx.Outputs[0].LockingScript, + Satoshis: fundingTx.Outputs[0].Satoshis, + })) + require.NoError(t, txT.AddP2PKHOutputFromAddress(address.AddressString, 49*100000000)) + require.NoError(t, txT.FillAllInputs(ctx, &unlocker.Getter{PrivateKey: privateKey})) + + _, _, err = realUtxoStore.SpendAndCreate(ctx, fundingTx, 0, utxostore.WithMinedBlockInfo(utxostore.MinedBlockInfo{BlockID: 0, BlockHeight: 0}), utxostore.WithCreateOnly()) + require.NoError(t, err) + + _, _, err = realUtxoStore.SpendAndCreate(ctx, txT, 0, utxostore.WithCreateOnly()) + require.NoError(t, err) + + subtree, err := subtreepkg.NewTreeByLeafCount(2) + require.NoError(t, err) + require.NoError(t, subtree.AddCoinbaseNode()) + require.NoError(t, subtree.AddNode(*txT.TxIDChainHash(), uint64(txT.Size()), 0)) //nolint:gosec + + subtreeMeta := subtreepkg.NewSubtreeMeta(subtree) + require.NoError(t, subtreeMeta.SetTxInpointsFromTx(txT)) + + subtreeBytes, err := subtree.Serialize() + require.NoError(t, err) + require.NoError(t, subtreeStore.Set(ctx, subtree.RootHash()[:], fileformat.FileTypeSubtree, subtreeBytes)) + + subtreeMetaBytes, err := subtreeMeta.Serialize() + require.NoError(t, err) + require.NoError(t, subtreeStore.Set(ctx, subtree.RootHash()[:], fileformat.FileTypeSubtreeMeta, subtreeMetaBytes)) + + const blockHeight = uint32(1) + + coinbaseTx := coinbaseAtHeight(t, blockHeight) + + replicated := subtree.Duplicate() + replicated.ReplaceRootNode(coinbaseTx.TxIDChainHash(), 0, uint64(coinbaseTx.Size())) //nolint:gosec + + hdr := minedBIP34Header(t, 4, tSettings.ChainCfgParams.GenesisHash, replicated.RootHash()) + + block, err := model.NewBlock(hdr, coinbaseTx, []*chainhash.Hash{subtree.RootHash()}, + uint64(subtree.Length()), uint64(coinbaseTx.Size()+txT.Size()), blockHeight, 0) //nolint:gosec + require.NoError(t, err) + + // A bare BlockValidation (no background setMined worker or subscription) keeps the run + // deterministic: the only setTxMinedStatus calls are the ones this test makes. The store writes + // genesis unmined, and ValidateBlock waits for the parent to be mined. + require.NoError(t, blockchainClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, false)) + + bv := &BlockValidation{ + logger: logger, + settings: tSettings, + blockchainClient: blockchainClient, + subtreeStore: subtreeStore, + txStore: txStore, + utxoStore: realUtxoStore, + subtreeValidationClient: subtreeValidationClient, + lastValidatedBlocks: expiringmap.New[chainhash.Hash, *model.Block](2 * time.Minute), + blockExistsCache: expiringmap.New[chainhash.Hash, bool](120 * time.Minute), + subtreeExistsCache: expiringmap.New[chainhash.Hash, bool](10 * time.Minute), + blockHashesCurrentlyValidated: txmap.NewSwissMap(0), + blocksCurrentlyValidating: txmap.NewSyncedMap[chainhash.Hash, *validationResult](), + setMinedChan: make(chan *chainhash.Hash, 1), + revalidateBlockChan: make(chan revalidateBlockData, 1), + stats: gocore.NewStat("blockvalidation"), + } + defer bv.StopCaches() + + // Validate and store the block (mined_set=false, subtrees_set=true); no tagging has run yet. + require.NoError(t, bv.ValidateBlock(ctx, block, "test")) + + _, preMeta, err := blockchainClient.GetBlockHeader(ctx, block.Hash()) + require.NoError(t, err) + require.False(t, preMeta.MinedSet, "precondition: tagging has not run yet") + require.False(t, preMeta.Invalid, "precondition: block is valid when the run starts") + + blocking := &gap058BlockingStore{ + Store: realUtxoStore, + entered: make(chan struct{}), + release: make(chan struct{}), + } + bv.utxoStore = blocking + blocking.armed.Store(true) + + // The tagging run starts (it read invalid=false) and parks inside SetMinedMulti. + runDone := make(chan error, 1) + + go func() { + runDone <- bv.setTxMinedStatus(ctx, block.Hash()) + }() + + select { + case <-blocking.entered: + case <-time.After(10 * time.Second): + t.Fatal("precondition: the tagging run never reached SetMinedMulti") + } + + // While the run is in flight the block is invalidated. + _, err = blockchainClient.InvalidateBlock(ctx, block.Hash()) + require.NoError(t, err) + + _, midMeta, err := blockchainClient.GetBlockHeader(ctx, block.Hash()) + require.NoError(t, err) + require.True(t, midMeta.Invalid, "precondition: block is invalid while the run is parked") + require.False(t, midMeta.MinedSet, "precondition: InvalidateBlock cleared mined_set") + + // Release the parked run and let it finish. + close(blocking.release) + + select { + case runErr := <-runDone: + // nil, not the state error: a refused write is not a failed run and must not + // charge the setMined retry counter + require.NoError(t, runErr, "the refused run must return nil") + case <-time.After(20 * time.Second): + t.Fatal("the tagging run did not finish after release") + } + + _, endMeta, err := blockchainClient.GetBlockHeader(ctx, block.Hash()) + require.NoError(t, err) + require.True(t, endMeta.Invalid, "the block is still invalid at the end") + + endTags, err := realUtxoStore.Get(ctx, txT.TxIDChainHash()) + require.NoError(t, err) + + tagsPresent := len(endTags.BlockIDs) > 0 + + require.False(t, endMeta.MinedSet, "the refused write must leave mined_set=false so the sweep still lists the block") + + select { + case requeued := <-bv.setMinedChan: + require.Equal(t, *block.Hash(), *requeued, "the refused run must re-queue the block for an unset run") + default: + t.Fatal("the refused run did not re-queue the block on setMinedChan") + } + + require.False(t, endMeta.MinedSet && tagsPresent, + "GAP-BV-058: block is invalid, yet the in-flight run left mined_set=true with the transaction still tagged (BlockIDs=%v); the unconditional SetBlockMinedSet overwrote the invalidation and the sweep will never revisit it", endTags.BlockIDs) +} diff --git a/services/blockvalidation/header_context_requeue_test.go b/services/blockvalidation/header_context_requeue_test.go index bc6ffff328..af7e743a1b 100644 --- a/services/blockvalidation/header_context_requeue_test.go +++ b/services/blockvalidation/header_context_requeue_test.go @@ -204,7 +204,7 @@ func TestValidateBlock_HeaderContextRetryBudgetIsBounded(t *testing.T) { mockBlockchain.On("AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() mockBlockchain.On("InvalidateBlock", mock.Anything, mock.Anything).Return([]chainhash.Hash{}, nil).Maybe() mockBlockchain.On("SetBlockSubtreesSet", mock.Anything, mock.Anything).Return(nil).Maybe() - mockBlockchain.On("SetBlockMinedSet", mock.Anything, mock.Anything).Return(nil).Maybe() + mockBlockchain.On("SetBlockMinedSet", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() notificationChan := make(chan *blockchain_api.Notification, 1) mockBlockchain.On("Subscribe", mock.Anything, mock.Anything).Return(notificationChan, nil).Maybe() diff --git a/services/blockvalidation/header_run_walk_fallback_test.go b/services/blockvalidation/header_run_walk_fallback_test.go index d8644caf0b..382ec41162 100644 --- a/services/blockvalidation/header_run_walk_fallback_test.go +++ b/services/blockvalidation/header_run_walk_fallback_test.go @@ -149,7 +149,7 @@ func TestValidateBlock_UnusableHeaderRunIsRebuiltByHashWalk(t *testing.T) { mockBlockchain.On("SetBlockSubtreesSet", mock.Anything, mock.Anything).Return(nil).Maybe() mockBlockchain.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything). Return([]uint32{2, 1, 0}, nil).Maybe() - mockBlockchain.On("SetBlockMinedSet", mock.Anything, mock.Anything).Return(nil).Maybe() + mockBlockchain.On("SetBlockMinedSet", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() mockBlockchain.On("InvalidateBlock", mock.Anything, mock.Anything).Return([]chainhash.Hash{}, nil).Maybe() mockBlockchain.On("CheckBlockIsInCurrentChain", mock.Anything, mock.Anything).Return(true, nil).Maybe() diff --git a/services/blockvalidation/invalid_block_reconsider_test.go b/services/blockvalidation/invalid_block_reconsider_test.go index b626482e12..77f836922a 100644 --- a/services/blockvalidation/invalid_block_reconsider_test.go +++ b/services/blockvalidation/invalid_block_reconsider_test.go @@ -102,7 +102,7 @@ func TestValidateBlock_ParentInvalid_ChildKeepsFullBodyAndReconsiders(t *testing // Reconsider the parent. RevalidateBlock clears mined_set, so re-stamp it the way the setMined // worker would; the child's validation waits on the parent's mined state. require.NoError(t, client.RevalidateBlock(ctx, parent.Hash())) - require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash())) + require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash(), false)) _, parentMeta, err := client.GetBlockHeader(ctx, parent.Hash()) require.NoError(t, err) diff --git a/services/legacy/netsync/block_park_policy_test.go b/services/legacy/netsync/block_park_policy_test.go index 3b9a7bebc5..326f199d4e 100644 --- a/services/legacy/netsync/block_park_policy_test.go +++ b/services/legacy/netsync/block_park_policy_test.go @@ -290,7 +290,7 @@ func TestSyncManager_AParkedBlockSurvivesAParentThatIsSlowToBeMarkedMined(t *tes // setTxMined finishes for the parent, and the next sweep pass resubmits the // kept block. - require.NoError(t, h.chain.SetBlockMinedSet(h.sm.ctx, &parent)) + require.NoError(t, h.chain.SetBlockMinedSet(h.sm.ctx, &parent, false)) h.sm.sweepParkedBlocks(time.Now().Add(2*parkStuckThreshold + time.Second)) diff --git a/services/legacy/netsync/legacy_validation_stack_test.go b/services/legacy/netsync/legacy_validation_stack_test.go index 20ff2af9c3..0a598a24c6 100644 --- a/services/legacy/netsync/legacy_validation_stack_test.go +++ b/services/legacy/netsync/legacy_validation_stack_test.go @@ -118,7 +118,7 @@ func newLegacyValidationStack(t *testing.T, s *settings.Settings) *legacyValidat chain, err := blockchain.NewLocalClient(logger, s, chainStore, subtrees, utxos) require.NoError(t, err) - require.NoError(t, chain.SetBlockMinedSet(ctx, s.ChainCfgParams.GenesisHash)) + require.NoError(t, chain.SetBlockMinedSet(ctx, s.ChainCfgParams.GenesisHash, false)) validate, err := validator.New(ctx, logger, s, utxos, nil, nil, nil, nil, chain) require.NoError(t, err) diff --git a/stores/blockchain/Interface.go b/stores/blockchain/Interface.go index 799e3ceb53..f5dbadbb92 100644 --- a/stores/blockchain/Interface.go +++ b/stores/blockchain/Interface.go @@ -369,12 +369,14 @@ type Store interface { SetState(ctx context.Context, key string, data []byte) error GetBlockIsMined(ctx context.Context, blockHash *chainhash.Hash) (bool, error) - // SetBlockMinedSet marks a block as mined. + // SetBlockMinedSet marks a block as mined, but only while its invalid flag + // still equals expectInvalid. // Parameters: // - ctx: Context for the operation // - blockHash: Hash of the block to mark - // Returns: Any error encountered - SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error + // - expectInvalid: The block's invalid flag when the caller's tagging run started + // Returns: errors.ErrStateError if the invalid flag has changed, else any error encountered + SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error // ClearBlockMinedSet resets the mined_set flag to false for a block, // triggering background job processing to update transaction states. diff --git a/stores/blockchain/mock.go b/stores/blockchain/mock.go index 5703df5dfc..3b8bebd09a 100644 --- a/stores/blockchain/mock.go +++ b/stores/blockchain/mock.go @@ -723,7 +723,7 @@ func (m *MockStore) GetBlockIsMined(_ context.Context, blockHash *chainhash.Hash return isMined, nil } -func (m *MockStore) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { +func (m *MockStore) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { panic(implementMe) } diff --git a/stores/blockchain/sql/RevalidateBlock_test.go b/stores/blockchain/sql/RevalidateBlock_test.go index 9e48d6f879..83ab4a99b3 100644 --- a/stores/blockchain/sql/RevalidateBlock_test.go +++ b/stores/blockchain/sql/RevalidateBlock_test.go @@ -66,7 +66,7 @@ func TestSQL_RevalidateBlock(t *testing.T) { assert.False(t, mined_set) // mined_set is set to false when invalidating to trigger tx unsetting // reset the mined_set to true, the revalidation should set it back to false - err = s.SetBlockMinedSet(t.Context(), block3.Hash()) + err = s.SetBlockMinedSet(t.Context(), block3.Hash(), true) require.NoError(t, err) err = s.RevalidateBlock(context.Background(), block3.Hash()) diff --git a/stores/blockchain/sql/SetBlockMinedSet.go b/stores/blockchain/sql/SetBlockMinedSet.go index db51505530..0ec0be2b7e 100644 --- a/stores/blockchain/sql/SetBlockMinedSet.go +++ b/stores/blockchain/sql/SetBlockMinedSet.go @@ -2,13 +2,23 @@ package sql import ( "context" + "database/sql" "github.com/bsv-blockchain/go-bt/v2/chainhash" "github.com/bsv-blockchain/teranode/errors" ) -func (s *SQL) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { - s.logger.Debugf("SetBlockMinedSet %s", blockHash.String()) +// SetBlockMinedSet sets mined_set = true, but only while the block's invalid flag +// still equals expectInvalid, the value the caller's tagging run started from. +// +// A tagging run (BlockValidation.setTxMinedStatus) reads invalid once, tags or +// untags the block's transactions, then calls this. If InvalidateBlock or +// RevalidateBlock ran in between, they cleared mined_set so the block would be +// processed again; an unconditional write here would undo that and the block +// would never be revisited. In that case the row is left untouched and an +// ErrStateError is returned, so the caller can re-queue the block. +func (s *SQL) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { + s.logger.Debugf("SetBlockMinedSet %s (expectInvalid=%t)", blockHash.String(), expectInvalid) // Invalidate response cache to ensure cached blocks reflect updated mined_set field defer s.ResetResponseCache() @@ -17,17 +27,27 @@ func (s *SQL) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) e UPDATE blocks SET mined_set = true WHERE hash = $1 + AND invalid = $2 ` - res, err := s.db.ExecContext(ctx, q, blockHash.CloneBytes()) + res, err := s.db.ExecContext(ctx, q, blockHash.CloneBytes(), expectInvalid) if err != nil { return errors.NewStorageError("error updating block mined_set", err) } - // check if the block was updated - if rows, _ := res.RowsAffected(); rows <= 0 { - return errors.NewStorageError("block %s mined_set was not updated", blockHash.String()) + if rows, _ := res.RowsAffected(); rows > 0 { + return nil } - return nil + // Nothing updated: either the block is missing or its invalid flag moved. + var invalid bool + if err = s.db.QueryRowContext(ctx, `SELECT invalid FROM blocks WHERE hash = $1`, blockHash.CloneBytes()).Scan(&invalid); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return errors.NewStorageError("block %s mined_set was not updated", blockHash.String()) + } + + return errors.NewStorageError("error reading block %s invalid flag", blockHash.String(), err) + } + + return errors.NewStateError("block %s mined_set not updated: invalid is %t, tagging run expected %t", blockHash.String(), invalid, expectInvalid) } diff --git a/stores/blockchain/sql/SetBlockMinedSet_test.go b/stores/blockchain/sql/SetBlockMinedSet_test.go new file mode 100644 index 0000000000..65f75b3b04 --- /dev/null +++ b/stores/blockchain/sql/SetBlockMinedSet_test.go @@ -0,0 +1,175 @@ +package sql + +import ( + "context" + "fmt" + "net/url" + "testing" + "time" + + "github.com/bsv-blockchain/go-bt/v2/chainhash" + "github.com/bsv-blockchain/teranode/errors" + "github.com/bsv-blockchain/teranode/stores/blockchain/options" + "github.com/bsv-blockchain/teranode/ulogger" + "github.com/bsv-blockchain/teranode/util/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/testcontainers/testcontainers-go" + "github.com/testcontainers/testcontainers-go/modules/postgres" + "github.com/testcontainers/testcontainers-go/wait" +) + +// TestSQL_SetBlockMinedSet covers the conditional write on sqlite: mined_set is only set while +// the block's invalid flag still equals the value the caller's tagging run started from. +func TestSQL_SetBlockMinedSet(t *testing.T) { + testSetBlockMinedSet(t, func(t *testing.T) *url.URL { + storeURL, err := url.Parse("sqlitememory:///") + require.NoError(t, err) + + return storeURL + }) +} + +// TestSQL_SetBlockMinedSet_Postgres runs the same cases against real PostgreSQL, where the +// boolean bind in "invalid = $2" has to compare against a BOOLEAN column; sqlite stores it as +// an integer and cannot prove that. +func TestSQL_SetBlockMinedSet_Postgres(t *testing.T) { + ctx := context.Background() + + pgContainer, err := postgres.Run(ctx, + "postgres:13", + postgres.WithDatabase("testdb"), + postgres.WithUsername("testuser"), + postgres.WithPassword("testpass"), + testcontainers.WithWaitStrategy( + wait.ForLog("database system is ready to accept connections"). + WithOccurrence(2). + WithStartupTimeout(5*time.Minute), + ), + ) + test.SkipIfContainerUnavailable(t, err) + + defer func() { + require.NoError(t, pgContainer.Terminate(ctx)) + }() + + connStr, err := pgContainer.ConnectionString(ctx, "sslmode=disable") + require.NoError(t, err) + + adminURL, err := url.Parse(connStr) + require.NoError(t, err) + + admin, err := New(ulogger.TestLogger{}, adminURL, test.CreateBaseTestSettings(t)) + require.NoError(t, err) + + defer admin.Close(ctx) + + dbCount := 0 + + // each case gets its own database, so the shared block fixtures never collide + testSetBlockMinedSet(t, func(t *testing.T) *url.URL { + dbCount++ + dbName := fmt.Sprintf("minedset%d", dbCount) + + _, err := admin.db.ExecContext(ctx, "CREATE DATABASE "+dbName) + require.NoError(t, err) + + caseURL := *adminURL + caseURL.Path = "/" + dbName + + return &caseURL + }) +} + +func testSetBlockMinedSet(t *testing.T, newStoreURL func(t *testing.T) *url.URL) { + tSettings := test.CreateBaseTestSettings(t) + + setup := func(t *testing.T) *SQL { + s, err := New(ulogger.TestLogger{}, newStoreURL(t), tSettings) + require.NoError(t, err) + + t.Cleanup(func() { _ = s.Close(context.Background()) }) + + _, _, err = s.StoreBlock(context.Background(), block1, "", options.WithMinedSet(true)) + require.NoError(t, err) + + _, _, err = s.StoreBlock(context.Background(), block2, "") + require.NoError(t, err) + + return s + } + + flags := func(t *testing.T, s *SQL, hash *chainhash.Hash) (invalid, minedSet bool) { + err := s.db.QueryRowContext(context.Background(), + `SELECT invalid, mined_set FROM blocks WHERE hash = $1`, hash.CloneBytes()).Scan(&invalid, &minedSet) + require.NoError(t, err) + + return invalid, minedSet + } + + t.Run("sets mined_set when invalid is unchanged", func(t *testing.T) { + s := setup(t) + + require.NoError(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), false)) + + invalid, minedSet := flags(t, s, block2.Hash()) + assert.False(t, invalid) + assert.True(t, minedSet) + }) + + t.Run("refuses after the block was invalidated mid-run", func(t *testing.T) { + s := setup(t) + + // a tagging run started while the block was valid, then the block was invalidated + _, err := s.InvalidateBlock(context.Background(), block2.Hash()) + require.NoError(t, err) + + err = s.SetBlockMinedSet(context.Background(), block2.Hash(), false) + require.Error(t, err) + assert.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) + + invalid, minedSet := flags(t, s, block2.Hash()) + assert.True(t, invalid) + assert.False(t, minedSet, "the invalidation's mined_set=false must survive, so the block is processed again") + }) + + t.Run("refuses after the block was revalidated mid-run", func(t *testing.T) { + s := setup(t) + + // an untagging run started while the block was invalid, then the block was revalidated + _, err := s.InvalidateBlock(context.Background(), block2.Hash()) + require.NoError(t, err) + + require.NoError(t, s.RevalidateBlock(context.Background(), block2.Hash())) + + err = s.SetBlockMinedSet(context.Background(), block2.Hash(), true) + require.Error(t, err) + assert.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) + + invalid, minedSet := flags(t, s, block2.Hash()) + assert.False(t, invalid) + assert.False(t, minedSet, "the revalidation's mined_set=false must survive, so the block is tagged again") + }) + + t.Run("sets mined_set on an invalid block for an untagging run", func(t *testing.T) { + s := setup(t) + + _, err := s.InvalidateBlock(context.Background(), block2.Hash()) + require.NoError(t, err) + + require.NoError(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), true)) + + invalid, minedSet := flags(t, s, block2.Hash()) + assert.True(t, invalid) + assert.True(t, minedSet) + }) + + t.Run("unknown block is a storage error, not a state error", func(t *testing.T) { + s := setup(t) + + err := s.SetBlockMinedSet(context.Background(), block3.Hash(), false) + require.Error(t, err) + assert.True(t, errors.Is(err, errors.ErrStorageError), "expected ErrStorageError, got %v", err) + assert.False(t, errors.Is(err, errors.ErrStateError)) + }) +} From d37b022c44cc3619ecb75bb63eeeee78d39c8e48 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 21 Oct 2026 12:03:37 +0100 Subject: [PATCH 2/5] test(blockchain): use require for the ErrStateError round-trip assertions in TestClientSetBlockMinedSet The repo's test convention is testify require, not assert; the two assertions added for the refused-write case now stop the subtest on failure. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UJ2B4FENo6G5s2TDXXvdKs --- services/blockchain/client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/blockchain/client_test.go b/services/blockchain/client_test.go index 00f4ffdd87..0e597700ed 100644 --- a/services/blockchain/client_test.go +++ b/services/blockchain/client_test.go @@ -4397,8 +4397,8 @@ func TestClientSetBlockMinedSet(t *testing.T) { err := c.SetBlockMinedSet(ctx, &blockHash, true) require.Error(t, err) - assert.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) - assert.True(t, c.client.(*mockBlockClient).lastSetBlockMinedSetReq.ExpectInvalid, "expectInvalid must reach the request") + require.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) + require.True(t, c.client.(*mockBlockClient).lastSetBlockMinedSetReq.ExpectInvalid, "expectInvalid must reach the request") }) } From e56bd60b9db1fa3bbdcd3650da4a39ccda630d34 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Tue, 27 Oct 2026 11:15:00 +0000 Subject: [PATCH 3/5] fix(blockvalidation): guard mined_set on a per-block generation, not the invalid flag The guard on invalid missed two cases. ClearBlockMinedSet resets mined_set without touching invalid, so a run that started before a catchup reset could still write mined_set=true over it. An invalidate then revalidate inside one run also put invalid back to its starting value, so the stale result landed. blocks gains mined_set_gen (default 0, added on startup). ClearBlockMinedSet, InvalidateBlock and RevalidateBlock bump it. GetBlockHeader returns it, the tagging run captures it before any work, and SetBlockMinedSet only writes while it is unchanged. A moved generation is still ERR_STATE_ERROR, so the run re-queues without charging a retry. The run also re-queues when the invalid flag the worker passed in differs from the one it reads itself, since the generation is captured after that gap. On the zero-rows path the RowsAffected error is now checked, and an unchanged generation returns a storage error, so a real storage fault is charged retries and cannot loop for free. The request field is now optional uint64 mined_set_generation. A request without it, from an older blockvalidation, gets the old unconditional write, so mixed versions keep working. CHANGELOG entry, proto and reference docs and the set-tx-mined diagram are updated. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016HzSYZhvqpYZPHF3YjMa3F --- CHANGELOG.md | 16 ++ .../protobuf_docs/blockchainProto.md | 3 + .../services/blockchain_reference.md | 7 + .../block_validation_set_tx_mined.puml | 26 ++- .../block_validation_set_tx_mined.svg | 2 +- model/BlockHeaderMeta.go | 6 + .../blockassembly_system_test.go | 4 +- services/blockchain/Client.go | 12 +- services/blockchain/Interface.go | 11 +- services/blockchain/LocalClient.go | 4 +- services/blockchain/LocalClient_test.go | 2 +- services/blockchain/Server.go | 6 +- .../blockchain_api/blockchain_api.pb.go | 78 ++++---- .../blockchain_api/blockchain_api.proto | 10 +- services/blockchain/client_test.go | 31 +++- services/blockchain/mock.go | 8 +- services/blockchain/server_test.go | 25 ++- services/blockpersister/Server_test.go | 2 +- services/blockvalidation/BlockValidation.go | 41 ++++- .../blockvalidation/BlockValidation_test.go | 4 +- .../InvalidBlocksKafkaProducer_test.go | 4 +- .../checkpoint_difficulty_reconsider_test.go | 5 +- .../corrupt_escalation_test.go | 4 +- services/blockvalidation/gap_bv058_test.go | 172 ++++++++++++++---- .../invalid_block_reconsider_test.go | 5 +- .../legacy/netsync/block_park_policy_test.go | 2 +- .../netsync/legacy_validation_stack_test.go | 2 +- stores/blockchain/Interface.go | 11 +- stores/blockchain/mock.go | 2 +- stores/blockchain/sql/ClearBlockMinedSet.go | 4 +- stores/blockchain/sql/GetBlockHeader.go | 2 + stores/blockchain/sql/InvalidateBlock.go | 5 +- stores/blockchain/sql/RevalidateBlock.go | 5 +- stores/blockchain/sql/RevalidateBlock_test.go | 2 +- .../sql/SchemaCurrent_PostgreSQL_test.go | 43 +++++ stores/blockchain/sql/SetBlockMinedSet.go | 84 ++++++--- .../blockchain/sql/SetBlockMinedSet_test.go | 162 ++++++++++++++--- stores/blockchain/sql/sql.go | 37 ++++ 38 files changed, 668 insertions(+), 181 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a118c71c2c..575e4e2077 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,22 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea ### Breaking — behaviour changes on upgrade +- **`SetBlockMinedSet` can now refuse a write with `ERR_STATE_ERROR`.** The blockchain + store gains a `mined_set_gen` column on `blocks`, added on startup with default 0. + `ClearBlockMinedSet`, `InvalidateBlock` and `RevalidateBlock` bump it. + `SetBlockMinedSetRequest` gains an optional `mined_set_generation` field, and + `GetBlockHeaderResponse` gains `mined_set_generation`. When the request carries the + field, the write only lands if the generation has not moved since the caller read it. + Otherwise the call returns `ERR_STATE_ERROR`, and block validation re-queues the + block. This stops a set-mined run that was in flight during an invalidation or a + catchup reset from overwriting that reset. Mixed versions keep working, so there is + no required upgrade order. An older block validation service sends no generation, and + a new blockchain service writes for it unconditionally, as before. A new block + validation service talking to an older blockchain service has its field ignored, and + gets the old unconditional write. Either way the race stays open until both services + run the new version. A client that calls `SetBlockMinedSet` with the field set must + handle `ERR_STATE_ERROR`. (#2018) + - **This release removes four Aerospike pruner metrics.** External monitoring systems must remove queries for `utxo_pruner_parents_skipped_pruned_total`, `utxo_pruner_pruned_set_size`, `utxo_pruner_pruned_set_saturated`, and diff --git a/docs/references/protobuf_docs/blockchainProto.md b/docs/references/protobuf_docs/blockchainProto.md index 345403f8ee..e82582d1d2 100644 --- a/docs/references/protobuf_docs/blockchainProto.md +++ b/docs/references/protobuf_docs/blockchainProto.md @@ -278,6 +278,8 @@ swagger:model GetBlockHeaderResponse | subtrees_set | [bool](#bool) | | Subtrees status | | invalid | [bool](#bool) | | Validity status | | processed_at | [google.protobuf.Timestamp](#google-protobuf-Timestamp) | | Timestamp when block was processed | +| median_time_past | [uint32](#uint32) | | Median Time Past (MTP) - median of last 11 block timestamps (BIP113) | +| mined_set_generation | [uint64](#uint64) | | Bumped by every write that clears mined_set; SetBlockMinedSet compares against it | @@ -898,6 +900,7 @@ SetBlockMinedSetRequest marks a block as mined. | Field | Type | Label | Description | | ----- | ---- | ----- | ----------- | | blockHash | [bytes](#bytes) | | Hash of the block to mark as mined | +| mined_set_generation | [uint64](#uint64) | optional | The block's mined_set generation (GetBlockHeaderResponse.mined_set_generation) when the caller's tagging run started. Every writer that clears mined_set bumps it, so the write is refused with ERR_STATE_ERROR if it has changed since. Absent means a caller that predates the field: the write is unconditional, as it was before. | diff --git a/docs/references/services/blockchain_reference.md b/docs/references/services/blockchain_reference.md index f78e947688..7c587601f0 100644 --- a/docs/references/services/blockchain_reference.md +++ b/docs/references/services/blockchain_reference.md @@ -353,6 +353,13 @@ func (b *Blockchain) SetBlockMinedSet(ctx context.Context, req *blockchain_api.S Marks a block as mined in the blockchain, updating its status to indicate completion of the mining process. +The write is conditional when the request carries `mined_set_generation`: it only lands while the +block's `mined_set_gen` still equals that value. `ClearBlockMinedSet`, `InvalidateBlock` and +`RevalidateBlock` each clear `mined_set` and bump `mined_set_gen`, so a tagging run that started +before one of them is refused with `ERR_STATE_ERROR` instead of overwriting the reset. Block +validation re-queues the block on that error. A request without the field, as sent by a block +validation service that predates it, is the old unconditional write. + ### GetBlocksMinedNotSet ```go diff --git a/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml b/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml index 303b0bbdd1..8cf03e7da1 100644 --- a/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml +++ b/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml @@ -54,6 +54,16 @@ else Block not in cache deactivate Blockchain end +BlockValidation -> Blockchain: GetBlockHeader(ctx, blockHash) +activate Blockchain +Blockchain --> BlockValidation: BlockHeaderMeta (invalid, mined_set_generation) +deactivate Blockchain +note right of BlockValidation + Capture mined_set_generation before any work. + If invalid differs from the flag the worker passed in, + re-queue the block and stop. +end note + BlockValidation -> BlockValidation: Ensure all subtrees are loaded note right of BlockValidation Using fallbackGetFunc with subtreeValidationClient @@ -106,7 +116,21 @@ alt Block was cached BlockValidation -> BlockValidation: Delete from lastValidatedBlocks cache end -BlockValidation -> Blockchain: SetBlockMinedSet(ctx, blockHash) +BlockValidation -> Blockchain: SetBlockMinedSet(ctx, blockHash, mined_set_generation) +activate Blockchain +note right of Blockchain + UPDATE ... SET mined_set = true + WHERE hash = $1 AND mined_set_gen = $2 + ClearBlockMinedSet, InvalidateBlock and + RevalidateBlock bump mined_set_gen +end note +alt Generation unchanged + Blockchain --> BlockValidation: ok +else Generation moved during the run + Blockchain --> BlockValidation: ERR_STATE_ERROR + BlockValidation -> BVServer: Re-queue on setMinedChan (retry counter not charged) +end +deactivate Blockchain BlockValidation --> BVServer deactivate BlockValidation diff --git a/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.svg b/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.svg index a1fa85298f..e79f400fe1 100644 --- a/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.svg +++ b/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.svg @@ -1 +1 @@ -BlockchainBlockchainBlock Validation ServerBlock Validation ServerBlock ValidationBlock ValidationModelModelSubtree StoreSubtree StoreUTXO StoreUTXO StoreSendNotification(NotificationType_BlockSubtreesSet,blockHashBytes)Enqueue to setMinedChanBackground goroutine processes setMinedChansetTxMined(ctx, blockHash)Check block cache firstalt[Block in cache (lastValidatedBlocks)]Use cached block[Block not in cache]GetBlock(ctx, blockHash)BlockEnsure all subtrees are loadedUsing fallbackGetFunc with subtreeValidationClientif subtrees aren't already loadedloop[for each subtree hash in the block]Get(ctx, subtreeHash)SubtreeGetBlockHeaderIDs(ctx, blockHash, 2 * UTXO_retention_height)chainBlockIDs[]UpdateTxMinedStatus(ctx, logger, settings, utxoStore, block, blockID, chainBlockIDs)Handles batch processing of transactionsto mark them as minedPasses chain block IDs for duplicate detectionSetMinedMulti(ctx, txBatchHashes, blockID)SetMinedMulti also unsets the locked flag(second phase of two-phase commit)Returns map of tx hashes to block IDswhere they were already minedmap[txHash]blockIDs, erroralt[Transactions already mined on chain]ErrBlockInvalidInvalidateBlock(ctx, blockHash)Block contains transactionsalready mined on this chain[No duplicates]successalt[Block was cached]Delete from lastValidatedBlocks cacheSetBlockMinedSet(ctx, blockHash)Last Modified On: 01-Sep-2025 +BlockchainBlockchainBlock Validation ServerBlock Validation ServerBlock ValidationBlock ValidationModelModelSubtree StoreSubtree StoreUTXO StoreUTXO StoreSendNotification(NotificationType_BlockSubtreesSet,blockHashBytes)Enqueue to setMinedChanBackground goroutine processes setMinedChansetTxMined(ctx, blockHash)Check block cache firstalt[Block in cache (lastValidatedBlocks)]Use cached block[Block not in cache]GetBlock(ctx, blockHash)BlockGetBlockHeader(ctx, blockHash)BlockHeaderMeta (invalid, mined_set_generation)Capture mined_set_generation before any work.If invalid differs from the flag the worker passed in,re-queue the block and stop.Ensure all subtrees are loadedUsing fallbackGetFunc with subtreeValidationClientif subtrees aren't already loadedloop[for each subtree hash in the block]Get(ctx, subtreeHash)SubtreeGetBlockHeaderIDs(ctx, blockHash, 2 * UTXO_retention_height)chainBlockIDs[]UpdateTxMinedStatus(ctx, logger, settings, utxoStore, block, blockID, chainBlockIDs)Handles batch processing of transactionsto mark them as minedPasses chain block IDs for duplicate detectionSetMinedMulti(ctx, txBatchHashes, blockID)SetMinedMulti also unsets the locked flag(second phase of two-phase commit)Returns map of tx hashes to block IDswhere they were already minedmap[txHash]blockIDs, erroralt[Transactions already mined on chain]ErrBlockInvalidInvalidateBlock(ctx, blockHash)Block contains transactionsalready mined on this chain[No duplicates]successalt[Block was cached]Delete from lastValidatedBlocks cacheSetBlockMinedSet(ctx, blockHash, mined_set_generation)UPDATE ... SET mined_set = trueWHERE hash = $1 AND mined_set_gen = $2ClearBlockMinedSet, InvalidateBlock andRevalidateBlock bump mined_set_genalt[Generation unchanged]ok[Generation moved during the run]ERR_STATE_ERRORRe-queue on setMinedChan (retry counter not charged)Last Modified On: 27-Oct-2026 diff --git a/model/BlockHeaderMeta.go b/model/BlockHeaderMeta.go index 1925786785..59f5c49219 100644 --- a/model/BlockHeaderMeta.go +++ b/model/BlockHeaderMeta.go @@ -23,6 +23,12 @@ type BlockHeaderMeta struct { SubtreesSet bool `json:"subtrees_set"` // Whether the block has its subtrees set. Invalid bool `json:"invalid"` // Whether the block is marked as invalid. ProcessedAt *time.Time `json:"processed_at"` // Timestamp when the block was processed (nullable). + + // MinedSetGeneration counts the writes that cleared mined_set (ClearBlockMinedSet, + // InvalidateBlock, RevalidateBlock). A tagging run captures it at start and passes it + // to SetBlockMinedSet, which refuses the write if it has moved. Only GetBlockHeader + // fills it; it is not part of Bytes(), so list endpoints leave it zero. + MinedSetGeneration uint64 `json:"mined_set_generation"` } func (m *BlockHeaderMeta) Bytes() []byte { diff --git a/services/blockassembly/blockassembly_system_test.go b/services/blockassembly/blockassembly_system_test.go index 32e95ef04c..66106f2eed 100644 --- a/services/blockassembly/blockassembly_system_test.go +++ b/services/blockassembly/blockassembly_system_test.go @@ -1181,7 +1181,9 @@ func TestHandleReorgWithInvalidBlock_Integration(t *testing.T) { // so call SetBlockMinedSet directly — otherwise BA's reset() blocks in // waitForBlockMinedSet until its retry budget exhausts. for i := range invalidatedHashes { - require.NoError(t, ba.blockchainClient.SetBlockMinedSet(ctx, &invalidatedHashes[i], true), + _, invalidatedMeta, err := ba.blockchainClient.GetBlockHeader(ctx, &invalidatedHashes[i]) + require.NoError(t, err) + require.NoError(t, ba.blockchainClient.SetBlockMinedSet(ctx, &invalidatedHashes[i], invalidatedMeta.MinedSetGeneration), "failed to re-set mined_set on invalidated block %s", invalidatedHashes[i].String()) } diff --git a/services/blockchain/Client.go b/services/blockchain/Client.go index c5c21feea9..e4f447bec4 100644 --- a/services/blockchain/Client.go +++ b/services/blockchain/Client.go @@ -845,6 +845,8 @@ func (c *Client) GetBlockHeader(ctx context.Context, blockHash *chainhash.Hash) SubtreesSet: resp.SubtreesSet, Invalid: resp.Invalid, MedianTimePast: resp.MedianTimePast, + + MinedSetGeneration: resp.MinedSetGeneration, } if resp.ProcessedAt != nil { @@ -1683,15 +1685,15 @@ func (c *Client) GetBlockIsMined(ctx context.Context, blockHash *chainhash.Hash) // Parameters: // - ctx: Context for the operation with timeout and cancellation support // - blockHash: Hash of the block to mark as mined -// - expectInvalid: The block's invalid flag when the caller's tagging run started +// - minedSetGeneration: The block's mined_set generation when the caller's tagging run started // // Returns: -// - error: errors.ErrStateError if the block's invalid flag has changed, or any +// - error: errors.ErrStateError if the block's mined_set generation has moved, or any // other error encountered during the mining status update operation -func (c *Client) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { +func (c *Client) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, minedSetGeneration uint64) error { _, err := c.client.SetBlockMinedSet(ctx, &blockchain_api.SetBlockMinedSetRequest{ - BlockHash: blockHash[:], - ExpectInvalid: expectInvalid, + BlockHash: blockHash[:], + MinedSetGeneration: &minedSetGeneration, }) if err != nil { return errors.UnwrapGRPC(err) diff --git a/services/blockchain/Interface.go b/services/blockchain/Interface.go index 0dca737f29..a4c17de75e 100644 --- a/services/blockchain/Interface.go +++ b/services/blockchain/Interface.go @@ -602,14 +602,15 @@ type ClientI interface { // Parameters: // - ctx: Context for the operation with timeout and cancellation support // - blockHash: Hash of the block to mark as mined - // - expectInvalid: The block's invalid flag when the caller's tagging run started. - // The write is refused if the flag has changed since, because InvalidateBlock or - // RevalidateBlock cleared mined_set so the block is processed again. + // - minedSetGeneration: BlockHeaderMeta.MinedSetGeneration read from GetBlockHeader + // when the caller's tagging run started. The write is refused if it has moved since, + // because ClearBlockMinedSet, InvalidateBlock or RevalidateBlock cleared mined_set so + // the block is processed again. // // Returns: - // - errors.ErrStateError if the block's invalid flag no longer equals expectInvalid + // - errors.ErrStateError if the block's mined_set generation no longer equals minedSetGeneration // - Error if the operation fails, nil on success - SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error + SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, minedSetGeneration uint64) error // ClearBlockMinedSet resets the mined_set flag to false for a block. // diff --git a/services/blockchain/LocalClient.go b/services/blockchain/LocalClient.go index 28e6a97cbd..97f4bf3439 100644 --- a/services/blockchain/LocalClient.go +++ b/services/blockchain/LocalClient.go @@ -388,8 +388,8 @@ func (c *LocalClient) GetBlockIsMined(ctx context.Context, blockHash *chainhash. return c.store.GetBlockIsMined(ctx, blockHash) } -func (c *LocalClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { - return c.store.SetBlockMinedSet(ctx, blockHash, expectInvalid) +func (c *LocalClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, minedSetGeneration uint64) error { + return c.store.SetBlockMinedSet(ctx, blockHash, &minedSetGeneration) } func (c *LocalClient) ClearBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { diff --git a/services/blockchain/LocalClient_test.go b/services/blockchain/LocalClient_test.go index 684da3521c..a5f20b479b 100644 --- a/services/blockchain/LocalClient_test.go +++ b/services/blockchain/LocalClient_test.go @@ -370,7 +370,7 @@ func TestLocalClient_NilStorePanics(t *testing.T) { { name: "SetBlockMinedSet", fn: func() { - _ = client.SetBlockMinedSet(ctx, &blockHash, false) + _ = client.SetBlockMinedSet(ctx, &blockHash, 0) }, }, { diff --git a/services/blockchain/Server.go b/services/blockchain/Server.go index 67a596251a..51cef2a17f 100644 --- a/services/blockchain/Server.go +++ b/services/blockchain/Server.go @@ -2011,6 +2011,8 @@ func (b *Blockchain) GetBlockHeader(ctx context.Context, req *blockchain_api.Get Invalid: meta.Invalid, ProcessedAt: processedAt, MedianTimePast: meta.MedianTimePast, + + MinedSetGeneration: meta.MinedSetGeneration, }, nil } @@ -2978,7 +2980,9 @@ func (b *Blockchain) SetBlockMinedSet(ctx context.Context, req *blockchain_api.S return nil, errors.WrapGRPC(errors.NewInvalidArgumentError("[Blockchain][SetBlockMinedSet] hash must be %d bytes, got %d", chainhash.HashSize, len(req.BlockHash))) } - err = b.store.SetBlockMinedSet(ctx, blockHash, req.ExpectInvalid) + // An absent generation comes from a blockvalidation that predates it; keep the + // unconditional write it relied on rather than refuse every call during a rolling upgrade. + err = b.store.SetBlockMinedSet(ctx, blockHash, req.MinedSetGeneration) if err != nil { return nil, errors.WrapGRPC(err) } diff --git a/services/blockchain/blockchain_api/blockchain_api.pb.go b/services/blockchain/blockchain_api/blockchain_api.pb.go index 0bb5d9af35..7a61ec562f 100644 --- a/services/blockchain/blockchain_api/blockchain_api.pb.go +++ b/services/blockchain/blockchain_api/blockchain_api.pb.go @@ -2172,24 +2172,25 @@ func (x *RevalidateBlockRequest) GetBlockHash() []byte { // GetBlockHeaderResponse contains detailed block header information. type GetBlockHeaderResponse struct { - state protoimpl.MessageState `protogen:"open.v1"` - BlockHeader []byte `protobuf:"bytes,1,opt,name=blockHeader,proto3" json:"blockHeader,omitempty"` // Serialized block header - Id uint32 `protobuf:"varint,2,opt,name=id,proto3" json:"id,omitempty"` // Block identifier - Height uint32 `protobuf:"varint,3,opt,name=height,proto3" json:"height,omitempty"` // Block height - TxCount uint64 `protobuf:"varint,4,opt,name=tx_count,json=txCount,proto3" json:"tx_count,omitempty"` // Transaction count - SizeInBytes uint64 `protobuf:"varint,5,opt,name=size_in_bytes,json=sizeInBytes,proto3" json:"size_in_bytes,omitempty"` // Block size - Miner string `protobuf:"bytes,6,opt,name=miner,proto3" json:"miner,omitempty"` // Miner identifier - PeerId string `protobuf:"bytes,7,opt,name=peer_id,json=peerId,proto3" json:"peer_id,omitempty"` // Peer identifier - BlockTime uint32 `protobuf:"varint,8,opt,name=block_time,json=blockTime,proto3" json:"block_time,omitempty"` // Block timestamp - Timestamp uint32 `protobuf:"varint,9,opt,name=timestamp,proto3" json:"timestamp,omitempty"` // Processing timestamp - ChainWork []byte `protobuf:"bytes,10,opt,name=chain_work,json=chainWork,proto3" json:"chain_work,omitempty"` // Accumulated chain work - MinedSet bool `protobuf:"varint,11,opt,name=mined_set,json=minedSet,proto3" json:"mined_set,omitempty"` // Mined status - SubtreesSet bool `protobuf:"varint,12,opt,name=subtrees_set,json=subtreesSet,proto3" json:"subtrees_set,omitempty"` // Subtrees status - Invalid bool `protobuf:"varint,13,opt,name=invalid,proto3" json:"invalid,omitempty"` // Validity status - ProcessedAt *timestamppb.Timestamp `protobuf:"bytes,14,opt,name=processed_at,json=processedAt,proto3" json:"processed_at,omitempty"` // Timestamp when block was processed - MedianTimePast uint32 `protobuf:"varint,15,opt,name=median_time_past,json=medianTimePast,proto3" json:"median_time_past,omitempty"` // Median Time Past (MTP) - median of last 11 block timestamps (BIP113) - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + state protoimpl.MessageState `protogen:"open.v1"` + BlockHeader []byte `protobuf:"bytes,1,opt,name=blockHeader,proto3" json:"blockHeader,omitempty"` // Serialized block header + Id uint32 `protobuf:"varint,2,opt,name=id,proto3" json:"id,omitempty"` // Block identifier + Height uint32 `protobuf:"varint,3,opt,name=height,proto3" json:"height,omitempty"` // Block height + TxCount uint64 `protobuf:"varint,4,opt,name=tx_count,json=txCount,proto3" json:"tx_count,omitempty"` // Transaction count + SizeInBytes uint64 `protobuf:"varint,5,opt,name=size_in_bytes,json=sizeInBytes,proto3" json:"size_in_bytes,omitempty"` // Block size + Miner string `protobuf:"bytes,6,opt,name=miner,proto3" json:"miner,omitempty"` // Miner identifier + PeerId string `protobuf:"bytes,7,opt,name=peer_id,json=peerId,proto3" json:"peer_id,omitempty"` // Peer identifier + BlockTime uint32 `protobuf:"varint,8,opt,name=block_time,json=blockTime,proto3" json:"block_time,omitempty"` // Block timestamp + Timestamp uint32 `protobuf:"varint,9,opt,name=timestamp,proto3" json:"timestamp,omitempty"` // Processing timestamp + ChainWork []byte `protobuf:"bytes,10,opt,name=chain_work,json=chainWork,proto3" json:"chain_work,omitempty"` // Accumulated chain work + MinedSet bool `protobuf:"varint,11,opt,name=mined_set,json=minedSet,proto3" json:"mined_set,omitempty"` // Mined status + SubtreesSet bool `protobuf:"varint,12,opt,name=subtrees_set,json=subtreesSet,proto3" json:"subtrees_set,omitempty"` // Subtrees status + Invalid bool `protobuf:"varint,13,opt,name=invalid,proto3" json:"invalid,omitempty"` // Validity status + ProcessedAt *timestamppb.Timestamp `protobuf:"bytes,14,opt,name=processed_at,json=processedAt,proto3" json:"processed_at,omitempty"` // Timestamp when block was processed + MedianTimePast uint32 `protobuf:"varint,15,opt,name=median_time_past,json=medianTimePast,proto3" json:"median_time_past,omitempty"` // Median Time Past (MTP) - median of last 11 block timestamps (BIP113) + MinedSetGeneration uint64 `protobuf:"varint,16,opt,name=mined_set_generation,json=minedSetGeneration,proto3" json:"mined_set_generation,omitempty"` // Bumped by every write that clears mined_set; SetBlockMinedSet compares against it + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *GetBlockHeaderResponse) Reset() { @@ -2327,6 +2328,13 @@ func (x *GetBlockHeaderResponse) GetMedianTimePast() uint32 { return 0 } +func (x *GetBlockHeaderResponse) GetMinedSetGeneration() uint64 { + if x != nil { + return x.MinedSetGeneration + } + return 0 +} + // CheckBlockIsCurrentChainResponse indicates if blocks are in the main chain. type CheckBlockIsCurrentChainResponse struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -3507,11 +3515,14 @@ func (x *GetNextWorkRequiredResponse) GetBits() []byte { type SetBlockMinedSetRequest struct { state protoimpl.MessageState `protogen:"open.v1"` BlockHash []byte `protobuf:"bytes,1,opt,name=blockHash,proto3" json:"blockHash,omitempty"` // Hash of the mined block - // The block's invalid flag when the caller's tagging run started; the write is - // refused with ERR_STATE_ERROR if the flag has changed since. - ExpectInvalid bool `protobuf:"varint,2,opt,name=expectInvalid,proto3" json:"expectInvalid,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + // The block's mined_set generation (GetBlockHeaderResponse.mined_set_generation) + // when the caller's tagging run started. Every writer that clears mined_set bumps + // it, so the write is refused with ERR_STATE_ERROR if it has changed since. + // Absent means a caller that predates the field: the write is unconditional, as + // it was before. + MinedSetGeneration *uint64 `protobuf:"varint,2,opt,name=mined_set_generation,json=minedSetGeneration,proto3,oneof" json:"mined_set_generation,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *SetBlockMinedSetRequest) Reset() { @@ -3551,11 +3562,11 @@ func (x *SetBlockMinedSetRequest) GetBlockHash() []byte { return nil } -func (x *SetBlockMinedSetRequest) GetExpectInvalid() bool { - if x != nil { - return x.ExpectInvalid +func (x *SetBlockMinedSetRequest) GetMinedSetGeneration() uint64 { + if x != nil && x.MinedSetGeneration != nil { + return *x.MinedSetGeneration } - return false + return 0 } type ClearBlockMinedSetRequest struct { @@ -7531,7 +7542,7 @@ const file_services_blockchain_blockchain_api_blockchain_api_proto_rawDesc = "" "\x17InvalidateBlockResponse\x12,\n" + "\x11invalidatedBlocks\x18\x01 \x03(\fR\x11invalidatedBlocks\"6\n" + "\x16RevalidateBlockRequest\x12\x1c\n" + - "\tblockHash\x18\x01 \x01(\fR\tblockHash\"\xef\x03\n" + + "\tblockHash\x18\x01 \x01(\fR\tblockHash\"\xa1\x04\n" + "\x16GetBlockHeaderResponse\x12 \n" + "\vblockHeader\x18\x01 \x01(\fR\vblockHeader\x12\x0e\n" + "\x02id\x18\x02 \x01(\rR\x02id\x12\x16\n" + @@ -7550,7 +7561,8 @@ const file_services_blockchain_blockchain_api_blockchain_api_proto_rawDesc = "" "\fsubtrees_set\x18\f \x01(\bR\vsubtreesSet\x12\x18\n" + "\ainvalid\x18\r \x01(\bR\ainvalid\x12=\n" + "\fprocessed_at\x18\x0e \x01(\v2\x1a.google.protobuf.TimestampR\vprocessedAt\x12(\n" + - "\x10median_time_past\x18\x0f \x01(\rR\x0emedianTimePast\"V\n" + + "\x10median_time_past\x18\x0f \x01(\rR\x0emedianTimePast\x120\n" + + "\x14mined_set_generation\x18\x10 \x01(\x04R\x12minedSetGeneration\"V\n" + " CheckBlockIsCurrentChainResponse\x122\n" + "\x14isPartOfCurrentChain\x18\x01 \x01(\bR\x14isPartOfCurrentChain\"^\n" + "\"CheckBlockIsAncestorOfBlockRequest\x12\x1a\n" + @@ -7619,10 +7631,11 @@ const file_services_blockchain_blockchain_api_blockchain_api_proto_rawDesc = "" "\x11previousBlockHash\x18\x01 \x01(\fR\x11previousBlockHash\x12*\n" + "\x10currentBlockTime\x18\x02 \x01(\x03R\x10currentBlockTime\"1\n" + "\x1bGetNextWorkRequiredResponse\x12\x12\n" + - "\x04bits\x18\x01 \x01(\fR\x04bits\"]\n" + + "\x04bits\x18\x01 \x01(\fR\x04bits\"\x87\x01\n" + "\x17SetBlockMinedSetRequest\x12\x1c\n" + - "\tblockHash\x18\x01 \x01(\fR\tblockHash\x12$\n" + - "\rexpectInvalid\x18\x02 \x01(\bR\rexpectInvalid\"9\n" + + "\tblockHash\x18\x01 \x01(\fR\tblockHash\x125\n" + + "\x14mined_set_generation\x18\x02 \x01(\x04H\x00R\x12minedSetGeneration\x88\x01\x01B\x17\n" + + "\x15_mined_set_generation\"9\n" + "\x19ClearBlockMinedSetRequest\x12\x1c\n" + "\tblockHash\x18\x01 \x01(\fR\tblockHash\">\n" + "\x1cGetBlocksMinedNotSetResponse\x12\x1e\n" + @@ -8404,6 +8417,7 @@ func file_services_blockchain_blockchain_api_blockchain_api_proto_init() { if File_services_blockchain_blockchain_api_blockchain_api_proto != nil { return } + file_services_blockchain_blockchain_api_blockchain_api_proto_msgTypes[63].OneofWrappers = []any{} type x struct{} out := protoimpl.TypeBuilder{ File: protoimpl.DescBuilder{ diff --git a/services/blockchain/blockchain_api/blockchain_api.proto b/services/blockchain/blockchain_api/blockchain_api.proto index 41d26f2503..ddd08cc3b0 100644 --- a/services/blockchain/blockchain_api/blockchain_api.proto +++ b/services/blockchain/blockchain_api/blockchain_api.proto @@ -481,6 +481,7 @@ message GetBlockHeaderResponse { bool invalid = 13; // Validity status google.protobuf.Timestamp processed_at = 14; // Timestamp when block was processed uint32 median_time_past = 15; // Median Time Past (MTP) - median of last 11 block timestamps (BIP113) + uint64 mined_set_generation = 16; // Bumped by every write that clears mined_set; SetBlockMinedSet compares against it } // CheckBlockIsCurrentChainResponse indicates if blocks are in the main chain. @@ -618,9 +619,12 @@ message GetNextWorkRequiredResponse { // SetBlockMinedSetRequest marks a block as mined. message SetBlockMinedSetRequest { bytes blockHash = 1; // Hash of the mined block - // The block's invalid flag when the caller's tagging run started; the write is - // refused with ERR_STATE_ERROR if the flag has changed since. - bool expectInvalid = 2; + // The block's mined_set generation (GetBlockHeaderResponse.mined_set_generation) + // when the caller's tagging run started. Every writer that clears mined_set bumps + // it, so the write is refused with ERR_STATE_ERROR if it has changed since. + // Absent means a caller that predates the field: the write is unconditional, as + // it was before. + optional uint64 mined_set_generation = 2; } message ClearBlockMinedSetRequest { diff --git a/services/blockchain/client_test.go b/services/blockchain/client_test.go index 0e597700ed..037ffb4404 100644 --- a/services/blockchain/client_test.go +++ b/services/blockchain/client_test.go @@ -4365,7 +4365,7 @@ func TestClientSetBlockMinedSet(t *testing.T) { settings: tSettings, } - err := c.SetBlockMinedSet(ctx, &blockHash, false) + err := c.SetBlockMinedSet(ctx, &blockHash, 0) require.NoError(t, err) }) @@ -4379,7 +4379,7 @@ func TestClientSetBlockMinedSet(t *testing.T) { settings: tSettings, } - err := c.SetBlockMinedSet(ctx, &blockHash, false) + err := c.SetBlockMinedSet(ctx, &blockHash, 0) require.Error(t, err) assert.Contains(t, err.Error(), "grpc error") }) @@ -4389,16 +4389,37 @@ func TestClientSetBlockMinedSet(t *testing.T) { // still holds after the server's WrapGRPC and the client's UnwrapGRPC c := &Client{ client: &mockBlockClient{ - err: errors.WrapGRPC(errors.NewStateError("invalid flag changed")), + err: errors.WrapGRPC(errors.NewStateError("mined_set generation moved")), }, logger: logger, settings: tSettings, } - err := c.SetBlockMinedSet(ctx, &blockHash, true) + err := c.SetBlockMinedSet(ctx, &blockHash, 7) require.Error(t, err) require.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) - require.True(t, c.client.(*mockBlockClient).lastSetBlockMinedSetReq.ExpectInvalid, "expectInvalid must reach the request") + + req := c.client.(*mockBlockClient).lastSetBlockMinedSetReq + require.NotNil(t, req.MinedSetGeneration, "the generation must be sent as present, so the server applies the guard") + require.Equal(t, uint64(7), *req.MinedSetGeneration) + }) + + t.Run("generation zero is still sent as present", func(t *testing.T) { + // zero is a real generation (a block never reset); it must not read as "absent", + // which the server treats as an old caller and writes unconditionally + c := &Client{ + client: &mockBlockClient{ + responseSetBlockMinedSet: &emptypb.Empty{}, + }, + logger: logger, + settings: tSettings, + } + + require.NoError(t, c.SetBlockMinedSet(ctx, &blockHash, 0)) + + req := c.client.(*mockBlockClient).lastSetBlockMinedSetReq + require.NotNil(t, req.MinedSetGeneration) + require.Equal(t, uint64(0), *req.MinedSetGeneration) }) } diff --git a/services/blockchain/mock.go b/services/blockchain/mock.go index fa9a9a5f4e..bbc9b9e660 100644 --- a/services/blockchain/mock.go +++ b/services/blockchain/mock.go @@ -379,8 +379,8 @@ func (m *Mock) SetState(ctx context.Context, key string, data []byte) error { } // SetBlockMinedSet mocks the SetBlockMinedSet method -func (m *Mock) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { - args := m.Called(ctx, blockHash, expectInvalid) +func (m *Mock) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, minedSetGeneration uint64) error { + args := m.Called(ctx, blockHash, minedSetGeneration) return args.Error(0) } @@ -770,8 +770,8 @@ func (m *mockStoreGetSetBlockIsMined) GetBlockIsMined(ctx context.Context, hash return args.Bool(0), nil } -func (m *mockStoreGetSetBlockIsMined) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash, expectInvalid bool) error { - args := m.Called(ctx, hash, expectInvalid) +func (m *mockStoreGetSetBlockIsMined) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash, expectedGeneration *uint64) error { + args := m.Called(ctx, hash, expectedGeneration) return args.Error(0) } diff --git a/services/blockchain/server_test.go b/services/blockchain/server_test.go index 2e68c37ea2..72da071329 100644 --- a/services/blockchain/server_test.go +++ b/services/blockchain/server_test.go @@ -2543,7 +2543,7 @@ func TestSetBlockMinedSet(t *testing.T) { t.Run("error - store returns error", func(t *testing.T) { hash := chainhash.DoubleHashH([]byte("error-case")) mockStore := &mockStoreGetSetBlockIsMined{} - mockStore.On("SetBlockMinedSet", mock.Anything, &hash, false). + mockStore.On("SetBlockMinedSet", mock.Anything, &hash, (*uint64)(nil)). Return(errors.NewProcessingError("forced error")) server, _ := New(ctx, logger, tSettings, mockStore, nil) @@ -2561,7 +2561,7 @@ func TestSetBlockMinedSet(t *testing.T) { t.Run("success - block marked as mined", func(t *testing.T) { hash := chainhash.DoubleHashH([]byte("success-case")) mockStore := &mockStoreGetSetBlockIsMined{} - mockStore.On("SetBlockMinedSet", mock.Anything, &hash, false). + mockStore.On("SetBlockMinedSet", mock.Anything, &hash, (*uint64)(nil)). Return(nil) server, _ := New(ctx, logger, tSettings, mockStore, nil) @@ -2575,6 +2575,27 @@ func TestSetBlockMinedSet(t *testing.T) { require.NotNil(t, resp) mockStore.AssertExpectations(t) }) + + t.Run("a present generation reaches the store", func(t *testing.T) { + // the two cases above send no generation, as an older blockvalidation does, and the + // store gets nil (the unconditional write); a present one must arrive intact + hash := chainhash.DoubleHashH([]byte("generation-case")) + gen := uint64(3) + mockStore := &mockStoreGetSetBlockIsMined{} + mockStore.On("SetBlockMinedSet", mock.Anything, &hash, mock.MatchedBy(func(g *uint64) bool { + return g != nil && *g == gen + })).Return(nil) + + server, _ := New(ctx, logger, tSettings, mockStore, nil) + + resp, err := server.SetBlockMinedSet(ctx, &blockchain_api.SetBlockMinedSetRequest{ + BlockHash: hash.CloneBytes(), + MinedSetGeneration: &gen, + }) + require.NoError(t, err) + require.NotNil(t, resp) + mockStore.AssertExpectations(t) + }) } func TestGetBlocksMinedNotSet(t *testing.T) { diff --git a/services/blockpersister/Server_test.go b/services/blockpersister/Server_test.go index 1e4510d53b..22eef93af2 100644 --- a/services/blockpersister/Server_test.go +++ b/services/blockpersister/Server_test.go @@ -759,7 +759,7 @@ func (m *MockBlockchainClient) GetMedianTimePastRange(_ context.Context, fromHei func (m *MockBlockchainClient) SetState(ctx context.Context, key string, data []byte) error { return nil } -func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { +func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, minedSetGeneration uint64) error { return nil } func (m *MockBlockchainClient) ClearBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { diff --git a/services/blockvalidation/BlockValidation.go b/services/blockvalidation/BlockValidation.go index dcd85e5fee..1a42f662b3 100644 --- a/services/blockvalidation/BlockValidation.go +++ b/services/blockvalidation/BlockValidation.go @@ -1483,6 +1483,29 @@ func (u *BlockValidation) setTxMinedStatus(ctx context.Context, blockHash *chain return errors.NewServiceError("[setTxMined][%s] failed to get block header from blockchain", blockHash.String(), err) } + // Capture the generation before any work. Everything this run decides (tag or untag, + // on the longest chain or not) is read after this point, so a reset that lands later + // moves the generation and the final SetBlockMinedSet is refused. + minedSetGeneration := blockHeaderMeta.MinedSetGeneration + + // The caller read the invalid flag earlier, in its own GetBlockHeader. If an invalidate + // or revalidate landed between that read and this one, the generation above already + // counts it, so the guard cannot catch a run started on the stale flag. Re-queue + // instead; the next pick-up reads both together. + if startedInvalid := len(unsetMined) > 0 && unsetMined[0]; startedInvalid != blockHeaderMeta.Invalid { + u.logger.Warnf("[setTxMined][%s] block invalid flag changed before the run started (caller saw %t, now %t), re-queueing", blockHash.String(), startedInvalid, blockHeaderMeta.Invalid) + + // A block taken from lastValidatedBlocks may hold mmap-backed subtrees, which + // have no finalizer; release them before dropping the block. + if releaseErr := block.ReleaseSubtreeNodes(nil); releaseErr != nil { + u.logger.Warnf("[setTxMined][%s] failed closing subtrees before re-queue: %v", blockHash.String(), releaseErr) + } + + u.enqueueSetMined(blockHash) + + return nil + } + onLongestChain, err = u.blockchainClient.CheckBlockIsInCurrentChain(ctx, []uint32{blockHeaderMeta.ID}) if err != nil { return errors.NewServiceError("[setTxMined][%s] failed to check if block is on longest chain", blockHash.String(), err) @@ -1568,17 +1591,17 @@ func (u *BlockValidation) setTxMinedStatus(ctx context.Context, blockHash *chain u.logger.Warnf("[setTxMined][%s] failed closing subtrees after setting mined: %v", block.Hash().String(), releaseErr) } - // update block mined_set to true, but only if the block's invalid flag is still the one - // this run started from. InvalidateBlock and RevalidateBlock flip that flag and clear - // mined_set so the block is processed again; overwriting mined_set here would make the - // worker's MinedSet guard and the mined-not-set sweep both skip the block for good, - // leaving an invalid block tagged or a revalidated block untagged. - startedInvalid := len(unsetMined) > 0 && unsetMined[0] - if err = u.blockchainClient.SetBlockMinedSet(ctx, blockHash, startedInvalid); err != nil { + // update block mined_set to true, but only if the block's mined_set generation is still + // the one this run started from. ClearBlockMinedSet, InvalidateBlock and RevalidateBlock + // clear mined_set so the block is processed again, and bump the generation. Overwriting + // mined_set here would make the worker's MinedSet guard and the mined-not-set sweep both + // skip the block for good, leaving its transactions tagged for a chain state that no + // longer holds. + if err = u.blockchainClient.SetBlockMinedSet(ctx, blockHash, minedSetGeneration); err != nil { if errors.Is(err, errors.ErrStateError) { // Not a failure of this run: re-queue so the block is processed again under its - // new invalid flag, and return nil so the retry counter is not charged. - u.logger.Warnf("[setTxMined][%s] block invalid flag changed during the run, re-queueing: %v", blockHash.String(), err) + // new state, and return nil so the retry counter is not charged. + u.logger.Warnf("[setTxMined][%s] block mined_set was cleared during the run, re-queueing: %v", blockHash.String(), err) u.enqueueSetMined(blockHash) return nil diff --git a/services/blockvalidation/BlockValidation_test.go b/services/blockvalidation/BlockValidation_test.go index 4e55b27b82..da77a60cd1 100644 --- a/services/blockvalidation/BlockValidation_test.go +++ b/services/blockvalidation/BlockValidation_test.go @@ -241,13 +241,13 @@ func (t *trackingBlockchainClient) Subscribe(ctx context.Context, tag string) (c return t.ClientI.Subscribe(ctx, tag) } -func (t *trackingBlockchainClient) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash, expectInvalid bool) error { +func (t *trackingBlockchainClient) SetBlockMinedSet(ctx context.Context, hash *chainhash.Hash, minedSetGeneration uint64) error { if t.setMinedCalled != nil { t.setMinedOnce.Do(func() { close(t.setMinedCalled) }) } - return t.ClientI.SetBlockMinedSet(ctx, hash, expectInvalid) + return t.ClientI.SetBlockMinedSet(ctx, hash, minedSetGeneration) } func (m *MockSubtreeValidationClient) Health(ctx context.Context, checkLiveness bool) (int, string, error) { diff --git a/services/blockvalidation/InvalidBlocksKafkaProducer_test.go b/services/blockvalidation/InvalidBlocksKafkaProducer_test.go index 554a0910f0..1bca8f021e 100644 --- a/services/blockvalidation/InvalidBlocksKafkaProducer_test.go +++ b/services/blockvalidation/InvalidBlocksKafkaProducer_test.go @@ -400,8 +400,8 @@ func (m *MockBlockchainClient) SendNotification(ctx context.Context, notificatio } // SetBlockMinedSet implements the blockchain.ClientI interface -func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { - args := m.Called(ctx, blockHash, expectInvalid) +func (m *MockBlockchainClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, minedSetGeneration uint64) error { + args := m.Called(ctx, blockHash, minedSetGeneration) return args.Error(0) } diff --git a/services/blockvalidation/checkpoint_difficulty_reconsider_test.go b/services/blockvalidation/checkpoint_difficulty_reconsider_test.go index 8379ed9235..2dd699b0ef 100644 --- a/services/blockvalidation/checkpoint_difficulty_reconsider_test.go +++ b/services/blockvalidation/checkpoint_difficulty_reconsider_test.go @@ -113,7 +113,10 @@ func TestReconsider_BelowCheckpoint_RunsExpectedNBitsAndSucceeds(t *testing.T) { // Reconsider the ancestor. RevalidateBlock clears mined_set, so re-stamp it the way the // setMined worker would; the descendant's validation waits on the parent's mined state. require.NoError(t, client.RevalidateBlock(ctx, parent.Hash())) - require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash(), false)) + + _, revalidatedMeta, err := client.GetBlockHeader(ctx, parent.Hash()) + require.NoError(t, err) + require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash(), revalidatedMeta.MinedSetGeneration)) // The difficulty inputs must still be reachable after the invalidation, which is the whole // claim: the calculator walks parent_id and does not filter invalid rows. diff --git a/services/blockvalidation/corrupt_escalation_test.go b/services/blockvalidation/corrupt_escalation_test.go index 87f1f0d4af..a1ee3b2d79 100644 --- a/services/blockvalidation/corrupt_escalation_test.go +++ b/services/blockvalidation/corrupt_escalation_test.go @@ -261,7 +261,7 @@ func TestOptimisticCorrupt_ReValidationConvergesOnInvalidation(t *testing.T) { // waitForPreviousBlocksToBeProcessed gates on the parent being mined, and the store writes // genesis unmined; without this the optimistic AddBlock is never reached. - require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, false)) + require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, 0)) blockchainClient := &invalidateFailsFirstNClient{ClientI: realClient, failFirst: 1} @@ -867,7 +867,7 @@ func TestOptimisticCorrupt_StrikeGatedOnRevalidation(t *testing.T) { // waitForPreviousBlocksToBeProcessed gates on the parent being mined, and the store writes // genesis unmined; without this the optimistic AddBlock is never reached. - require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, false)) + require.NoError(t, realClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, 0)) // Height 1, so the model height, the store-derived height and the BIP34 coinbase-encoded // height all agree (StoreBlock re-derives the height from the parent chain). diff --git a/services/blockvalidation/gap_bv058_test.go b/services/blockvalidation/gap_bv058_test.go index c256241486..e6f28308db 100644 --- a/services/blockvalidation/gap_bv058_test.go +++ b/services/blockvalidation/gap_bv058_test.go @@ -57,22 +57,24 @@ func (s *gap058BlockingStore) SetMinedMulti(ctx context.Context, hashes []*chain return s.Store.SetMinedMulti(ctx, hashes, info) } -// TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation is the regression test for GAP-BV-058 -// (BV-COMMIT-005, BV-COMMIT-011, BV-REVALIDATE-003). -// -// A set-mined tagging run that is in flight when the block is invalidated must not leave the block -// invalid with mined_set=true and its transactions still tagged. InvalidateBlock clears mined_set so -// the block is processed again; an unconditional SetBlockMinedSet at the end of the run used to -// overwrite that, after which the setMined worker's MinedSet guard and the mined-not-set sweep both -// skipped the block for good. SetBlockMinedSet is now conditional on the invalid flag the run started -// from, and setTxMinedStatus re-queues the block when the write is refused. The race is made -// deterministic with a UTXO-store decorator that parks the tagging run inside SetMinedMulti while -// InvalidateBlock runs on the real sqlitememory store. -func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { +// gap058Fixture is a validated, stored block whose single transaction has not been tagged yet, +// with a bare BlockValidation over real sqlitememory blockchain and UTXO stores. +type gap058Fixture struct { + ctx context.Context + bv *BlockValidation + blockchainClient blockchain.ClientI + utxoStore utxostore.Store + block *model.Block + tx *bt.Tx +} + +func newGap058Fixture(t *testing.T) *gap058Fixture { + t.Helper() + initPrometheusMetrics() ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + t.Cleanup(cancel) logger := ulogger.TestLogger{} tSettings := test.CreateBaseTestSettings(t) @@ -153,7 +155,7 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { // A bare BlockValidation (no background setMined worker or subscription) keeps the run // deterministic: the only setTxMinedStatus calls are the ones this test makes. The store writes // genesis unmined, and ValidateBlock waits for the parent to be mined. - require.NoError(t, blockchainClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, false)) + require.NoError(t, blockchainClient.SetBlockMinedSet(ctx, tSettings.ChainCfgParams.GenesisHash, 0)) bv := &BlockValidation{ logger: logger, @@ -172,7 +174,7 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { revalidateBlockChan: make(chan revalidateBlockData, 1), stats: gocore.NewStat("blockvalidation"), } - defer bv.StopCaches() + t.Cleanup(bv.StopCaches) // Validate and store the block (mined_set=false, subtrees_set=true); no tagging has run yet. require.NoError(t, bv.ValidateBlock(ctx, block, "test")) @@ -182,19 +184,33 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { require.False(t, preMeta.MinedSet, "precondition: tagging has not run yet") require.False(t, preMeta.Invalid, "precondition: block is valid when the run starts") + return &gap058Fixture{ + ctx: ctx, + bv: bv, + blockchainClient: blockchainClient, + utxoStore: realUtxoStore, + block: block, + tx: txT, + } +} + +// runParkedTagging starts a tagging run, parks it inside SetMinedMulti, calls interfere while it +// is parked, then releases it and returns the run's error. +func (f *gap058Fixture) runParkedTagging(t *testing.T, interfere func()) error { + t.Helper() + blocking := &gap058BlockingStore{ - Store: realUtxoStore, + Store: f.utxoStore, entered: make(chan struct{}), release: make(chan struct{}), } - bv.utxoStore = blocking + f.bv.utxoStore = blocking blocking.armed.Store(true) - // The tagging run starts (it read invalid=false) and parks inside SetMinedMulti. runDone := make(chan error, 1) go func() { - runDone <- bv.setTxMinedStatus(ctx, block.Hash()) + runDone <- f.bv.setTxMinedStatus(f.ctx, f.block.Hash()) }() select { @@ -203,45 +219,123 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { t.Fatal("precondition: the tagging run never reached SetMinedMulti") } - // While the run is in flight the block is invalidated. - _, err = blockchainClient.InvalidateBlock(ctx, block.Hash()) - require.NoError(t, err) - - _, midMeta, err := blockchainClient.GetBlockHeader(ctx, block.Hash()) - require.NoError(t, err) - require.True(t, midMeta.Invalid, "precondition: block is invalid while the run is parked") - require.False(t, midMeta.MinedSet, "precondition: InvalidateBlock cleared mined_set") + interfere() - // Release the parked run and let it finish. close(blocking.release) select { case runErr := <-runDone: - // nil, not the state error: a refused write is not a failed run and must not - // charge the setMined retry counter - require.NoError(t, runErr, "the refused run must return nil") + return runErr case <-time.After(20 * time.Second): t.Fatal("the tagging run did not finish after release") } - _, endMeta, err := blockchainClient.GetBlockHeader(ctx, block.Hash()) + return nil +} + +// requireRequeued asserts the refused run put the block back on setMinedChan. +func (f *gap058Fixture) requireRequeued(t *testing.T) { + t.Helper() + + select { + case requeued := <-f.bv.setMinedChan: + require.Equal(t, *f.block.Hash(), *requeued, "the refused run must re-queue the block") + default: + t.Fatal("the refused run did not re-queue the block on setMinedChan") + } +} + +// TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation is the regression test for GAP-BV-058 +// (BV-COMMIT-005, BV-COMMIT-011, BV-REVALIDATE-003). +// +// A set-mined tagging run that is in flight when the block is invalidated must not leave the block +// invalid with mined_set=true and its transactions still tagged. InvalidateBlock clears mined_set so +// the block is processed again; an unconditional SetBlockMinedSet at the end of the run used to +// overwrite that, after which the setMined worker's MinedSet guard and the mined-not-set sweep both +// skipped the block for good. SetBlockMinedSet is now conditional on the mined_set generation the run +// started from, and setTxMinedStatus re-queues the block when the write is refused. The race is made +// deterministic with a UTXO-store decorator that parks the tagging run inside SetMinedMulti while +// InvalidateBlock runs on the real sqlitememory store. +func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { + f := newGap058Fixture(t) + ctx := f.ctx + + runErr := f.runParkedTagging(t, func() { + _, err := f.blockchainClient.InvalidateBlock(ctx, f.block.Hash()) + require.NoError(t, err) + + _, midMeta, err := f.blockchainClient.GetBlockHeader(ctx, f.block.Hash()) + require.NoError(t, err) + require.True(t, midMeta.Invalid, "precondition: block is invalid while the run is parked") + require.False(t, midMeta.MinedSet, "precondition: InvalidateBlock cleared mined_set") + }) + + // nil, not the state error: a refused write is not a failed run and must not + // charge the setMined retry counter + require.NoError(t, runErr, "the refused run must return nil") + + _, endMeta, err := f.blockchainClient.GetBlockHeader(ctx, f.block.Hash()) require.NoError(t, err) require.True(t, endMeta.Invalid, "the block is still invalid at the end") - endTags, err := realUtxoStore.Get(ctx, txT.TxIDChainHash()) + endTags, err := f.utxoStore.Get(ctx, f.tx.TxIDChainHash()) require.NoError(t, err) tagsPresent := len(endTags.BlockIDs) > 0 require.False(t, endMeta.MinedSet, "the refused write must leave mined_set=false so the sweep still lists the block") - select { - case requeued := <-bv.setMinedChan: - require.Equal(t, *block.Hash(), *requeued, "the refused run must re-queue the block for an unset run") - default: - t.Fatal("the refused run did not re-queue the block on setMinedChan") - } + f.requireRequeued(t) require.False(t, endMeta.MinedSet && tagsPresent, "GAP-BV-058: block is invalid, yet the in-flight run left mined_set=true with the transaction still tagged (BlockIDs=%v); the unconditional SetBlockMinedSet overwrote the invalidation and the sweep will never revisit it", endTags.BlockIDs) } + +// TestGapBV058_InFlightTaggingDoesNotOverwriteClearBlockMinedSet covers the reset that does not +// touch the invalid flag. Catchup calls ClearBlockMinedSet on a block that moved off the main chain. +// A run that started before, with onLongestChain=true, used to match a guard on invalid and write +// mined_set=true over the clear, after which the re-queued run skipped on MinedSet=true and the +// block's transactions stayed tagged as on the longest chain. The generation guard refuses it. +func TestGapBV058_InFlightTaggingDoesNotOverwriteClearBlockMinedSet(t *testing.T) { + f := newGap058Fixture(t) + ctx := f.ctx + + runErr := f.runParkedTagging(t, func() { + require.NoError(t, f.blockchainClient.ClearBlockMinedSet(ctx, f.block.Hash())) + }) + + require.NoError(t, runErr, "the refused run must return nil") + + _, endMeta, err := f.blockchainClient.GetBlockHeader(ctx, f.block.Hash()) + require.NoError(t, err) + require.False(t, endMeta.Invalid, "precondition: the clear left invalid untouched") + require.False(t, endMeta.MinedSet, "the run must not write mined_set=true over ClearBlockMinedSet") + + f.requireRequeued(t) +} + +// TestGapBV058_InvalidFlagMovedBeforeRunStarts covers the window between the worker's read of +// the invalid flag and the run's own GetBlockHeader. The generation is captured at the second read, +// so it already counts an invalidation that landed in between and cannot catch it. The run must +// notice the caller's flag disagrees with the one it read, and re-queue without tagging. +func TestGapBV058_InvalidFlagMovedBeforeRunStarts(t *testing.T) { + f := newGap058Fixture(t) + ctx := f.ctx + + // the worker read invalid=false, then the block was invalidated before the run started + _, err := f.blockchainClient.InvalidateBlock(ctx, f.block.Hash()) + require.NoError(t, err) + + require.NoError(t, f.bv.setTxMinedStatus(ctx, f.block.Hash(), false)) + + _, endMeta, err := f.blockchainClient.GetBlockHeader(ctx, f.block.Hash()) + require.NoError(t, err) + require.True(t, endMeta.Invalid) + require.False(t, endMeta.MinedSet, "a run started on the stale flag must not set mined_set") + + endTags, err := f.utxoStore.Get(ctx, f.tx.TxIDChainHash()) + require.NoError(t, err) + require.Empty(t, endTags.BlockIDs, "a run started on the stale flag must not tag the invalid block's transaction") + + f.requireRequeued(t) +} diff --git a/services/blockvalidation/invalid_block_reconsider_test.go b/services/blockvalidation/invalid_block_reconsider_test.go index 77f836922a..1c5a4bc46d 100644 --- a/services/blockvalidation/invalid_block_reconsider_test.go +++ b/services/blockvalidation/invalid_block_reconsider_test.go @@ -102,7 +102,10 @@ func TestValidateBlock_ParentInvalid_ChildKeepsFullBodyAndReconsiders(t *testing // Reconsider the parent. RevalidateBlock clears mined_set, so re-stamp it the way the setMined // worker would; the child's validation waits on the parent's mined state. require.NoError(t, client.RevalidateBlock(ctx, parent.Hash())) - require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash(), false)) + + _, revalidatedMeta, err := client.GetBlockHeader(ctx, parent.Hash()) + require.NoError(t, err) + require.NoError(t, client.SetBlockMinedSet(ctx, parent.Hash(), revalidatedMeta.MinedSetGeneration)) _, parentMeta, err := client.GetBlockHeader(ctx, parent.Hash()) require.NoError(t, err) diff --git a/services/legacy/netsync/block_park_policy_test.go b/services/legacy/netsync/block_park_policy_test.go index 326f199d4e..4dde256ebc 100644 --- a/services/legacy/netsync/block_park_policy_test.go +++ b/services/legacy/netsync/block_park_policy_test.go @@ -290,7 +290,7 @@ func TestSyncManager_AParkedBlockSurvivesAParentThatIsSlowToBeMarkedMined(t *tes // setTxMined finishes for the parent, and the next sweep pass resubmits the // kept block. - require.NoError(t, h.chain.SetBlockMinedSet(h.sm.ctx, &parent, false)) + require.NoError(t, h.chain.SetBlockMinedSet(h.sm.ctx, &parent, 0)) h.sm.sweepParkedBlocks(time.Now().Add(2*parkStuckThreshold + time.Second)) diff --git a/services/legacy/netsync/legacy_validation_stack_test.go b/services/legacy/netsync/legacy_validation_stack_test.go index 0a598a24c6..27ab98e3a3 100644 --- a/services/legacy/netsync/legacy_validation_stack_test.go +++ b/services/legacy/netsync/legacy_validation_stack_test.go @@ -118,7 +118,7 @@ func newLegacyValidationStack(t *testing.T, s *settings.Settings) *legacyValidat chain, err := blockchain.NewLocalClient(logger, s, chainStore, subtrees, utxos) require.NoError(t, err) - require.NoError(t, chain.SetBlockMinedSet(ctx, s.ChainCfgParams.GenesisHash, false)) + require.NoError(t, chain.SetBlockMinedSet(ctx, s.ChainCfgParams.GenesisHash, 0)) validate, err := validator.New(ctx, logger, s, utxos, nil, nil, nil, nil, chain) require.NoError(t, err) diff --git a/stores/blockchain/Interface.go b/stores/blockchain/Interface.go index f5dbadbb92..064bfe8c50 100644 --- a/stores/blockchain/Interface.go +++ b/stores/blockchain/Interface.go @@ -369,14 +369,15 @@ type Store interface { SetState(ctx context.Context, key string, data []byte) error GetBlockIsMined(ctx context.Context, blockHash *chainhash.Hash) (bool, error) - // SetBlockMinedSet marks a block as mined, but only while its invalid flag - // still equals expectInvalid. + // SetBlockMinedSet marks a block as mined. With a non-nil expectedGeneration the + // write only lands while the block's mined_set generation still equals it. // Parameters: // - ctx: Context for the operation // - blockHash: Hash of the block to mark - // - expectInvalid: The block's invalid flag when the caller's tagging run started - // Returns: errors.ErrStateError if the invalid flag has changed, else any error encountered - SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error + // - expectedGeneration: BlockHeaderMeta.MinedSetGeneration when the caller's tagging + // run started, or nil for the unconditional write older callers expect + // Returns: errors.ErrStateError if the generation has moved, else any error encountered + SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectedGeneration *uint64) error // ClearBlockMinedSet resets the mined_set flag to false for a block, // triggering background job processing to update transaction states. diff --git a/stores/blockchain/mock.go b/stores/blockchain/mock.go index 3b8bebd09a..3347cecf6b 100644 --- a/stores/blockchain/mock.go +++ b/stores/blockchain/mock.go @@ -723,7 +723,7 @@ func (m *MockStore) GetBlockIsMined(_ context.Context, blockHash *chainhash.Hash return isMined, nil } -func (m *MockStore) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { +func (m *MockStore) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectedGeneration *uint64) error { panic(implementMe) } diff --git a/stores/blockchain/sql/ClearBlockMinedSet.go b/stores/blockchain/sql/ClearBlockMinedSet.go index e514f81f92..46f50e0429 100644 --- a/stores/blockchain/sql/ClearBlockMinedSet.go +++ b/stores/blockchain/sql/ClearBlockMinedSet.go @@ -10,6 +10,8 @@ import ( // ClearBlockMinedSet resets mined_set to false for the specified block. // This triggers the background job to re-process transaction states. // Used during fork handling when blocks move from main chain to side chain. +// It also bumps mined_set_gen, so a tagging run that started before the clear +// has its SetBlockMinedSet refused instead of overwriting the reset. func (s *SQL) ClearBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { s.logger.Debugf("ClearBlockMinedSet %s", blockHash.String()) @@ -18,7 +20,7 @@ func (s *SQL) ClearBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) q := ` UPDATE blocks - SET mined_set = false + SET mined_set = false, mined_set_gen = mined_set_gen + 1 WHERE hash = $1 ` diff --git a/stores/blockchain/sql/GetBlockHeader.go b/stores/blockchain/sql/GetBlockHeader.go index 7e5f8bc5eb..8d71731190 100644 --- a/stores/blockchain/sql/GetBlockHeader.go +++ b/stores/blockchain/sql/GetBlockHeader.go @@ -134,6 +134,7 @@ func (s *SQL) GetBlockHeader(ctx context.Context, blockHash *chainhash.Hash) (*m ,b.inserted_at ,b.processed_at ,b.median_time_past + ,b.mined_set_gen FROM blocks b WHERE b.hash = $1 ` @@ -171,6 +172,7 @@ func (s *SQL) GetBlockHeader(ctx context.Context, blockHash *chainhash.Hash) (*m &insertedAt, &processedAt, &blockHeaderMeta.MedianTimePast, + &blockHeaderMeta.MinedSetGeneration, ); err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, nil, errors.NewBlockNotFoundError("error in GetBlockHeader", errors.ErrNotFound) diff --git a/stores/blockchain/sql/InvalidateBlock.go b/stores/blockchain/sql/InvalidateBlock.go index 36c58c734f..38c5b5639f 100644 --- a/stores/blockchain/sql/InvalidateBlock.go +++ b/stores/blockchain/sql/InvalidateBlock.go @@ -93,7 +93,8 @@ func (s *SQL) InvalidateBlock(ctx context.Context, blockHash *chainhash.Hash) (i // recursively update all children blocks to invalid in 1 query. // Also set mined_set = false (invalid blocks cannot be mined; this triggers - // block-validation to reset mining state) and on_main_chain = false so the + // block-validation to reset mining state), bump mined_set_gen so an in-flight + // tagging run cannot overwrite that reset, and on_main_chain = false so the // invalidated subtree is removed from the canonical chain in the same // transaction — no follow-up wide-window UPDATE needed. q := ` @@ -107,7 +108,7 @@ func (s *SQL) InvalidateBlock(ctx context.Context, blockHash *chainhash.Hash) (i INNER JOIN children c ON c.hash = b.previous_hash ) UPDATE blocks - SET invalid = true, mined_set = false, on_main_chain = false + SET invalid = true, mined_set = false, mined_set_gen = mined_set_gen + 1, on_main_chain = false WHERE id IN (SELECT id FROM children) RETURNING hash ` diff --git a/stores/blockchain/sql/RevalidateBlock.go b/stores/blockchain/sql/RevalidateBlock.go index 8b603414e3..4df80a8093 100644 --- a/stores/blockchain/sql/RevalidateBlock.go +++ b/stores/blockchain/sql/RevalidateBlock.go @@ -32,10 +32,11 @@ func (s *SQL) RevalidateBlock(ctx context.Context, blockHash *chainhash.Hash) er s.mainChainRebuilding.Add(1) defer s.mainChainRebuilding.Add(-1) - // Update the block to valid (not invalid) and clear the mined_set flag. + // Update the block to valid (not invalid) and clear the mined_set flag. Bump + // mined_set_gen so an in-flight tagging run cannot overwrite the reset. q := ` UPDATE blocks - SET invalid = false, mined_set = false + SET invalid = false, mined_set = false, mined_set_gen = mined_set_gen + 1 WHERE hash = $1 ` if _, err = s.db.ExecContext(ctx, q, blockHash.CloneBytes()); err != nil { diff --git a/stores/blockchain/sql/RevalidateBlock_test.go b/stores/blockchain/sql/RevalidateBlock_test.go index 83ab4a99b3..66310e7cd4 100644 --- a/stores/blockchain/sql/RevalidateBlock_test.go +++ b/stores/blockchain/sql/RevalidateBlock_test.go @@ -66,7 +66,7 @@ func TestSQL_RevalidateBlock(t *testing.T) { assert.False(t, mined_set) // mined_set is set to false when invalidating to trigger tx unsetting // reset the mined_set to true, the revalidation should set it back to false - err = s.SetBlockMinedSet(t.Context(), block3.Hash(), true) + err = s.SetBlockMinedSet(t.Context(), block3.Hash(), nil) require.NoError(t, err) err = s.RevalidateBlock(context.Background(), block3.Hash()) diff --git a/stores/blockchain/sql/SchemaCurrent_PostgreSQL_test.go b/stores/blockchain/sql/SchemaCurrent_PostgreSQL_test.go index b32c378431..a7a2e4459f 100644 --- a/stores/blockchain/sql/SchemaCurrent_PostgreSQL_test.go +++ b/stores/blockchain/sql/SchemaCurrent_PostgreSQL_test.go @@ -106,6 +106,49 @@ func TestIsBlockchainSchemaCurrent_MissingColumn(t *testing.T) { assert.False(t, current, "schema should not be current after column drop") } +// TestBlockchainSchema_AddsMinedSetGenToExistingBlocks covers the upgrade of a database that +// predates mined_set_gen: reopening the store must add the column, give existing rows 0, and +// leave the schema current, so a tagging run on an old block reads generation 0 and its +// SetBlockMinedSet still lands. +func TestBlockchainSchema_AddsMinedSetGenToExistingBlocks(t *testing.T) { + if testing.Short() { + t.Skip("skipping postgres-backed test in -short mode") + } + + dbURL, db := newSchemaTestDB(t) + + tSettings := test.CreateBaseTestSettings(t) + s, err := New(ulogger.TestLogger{}, dbURL, tSettings) + require.NoError(t, err) + + _, _, err = s.StoreBlock(context.Background(), block1, "") + require.NoError(t, err) + require.NoError(t, s.Close(context.Background())) + + // simulate a database written by a build without the column + _, err = db.Exec(`ALTER TABLE blocks DROP COLUMN mined_set_gen`) + require.NoError(t, err) + + current, err := isBlockchainSchemaCurrent(db, true) + require.NoError(t, err) + require.False(t, current, "a missing mined_set_gen must send the store through the DDL path") + + s, err = New(ulogger.TestLogger{}, dbURL, tSettings) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close(context.Background()) }) + + current, err = isBlockchainSchemaCurrent(db, true) + require.NoError(t, err) + require.True(t, current, "reopening the store must add mined_set_gen back") + + _, meta, err := s.GetBlockHeader(context.Background(), block1.Hash()) + require.NoError(t, err) + require.Equal(t, uint64(0), meta.MinedSetGeneration, "existing rows start at generation 0") + + gen := meta.MinedSetGeneration + require.NoError(t, s.SetBlockMinedSet(context.Background(), block1.Hash(), &gen)) +} + func TestIsBlockchainSchemaCurrent_MissingReservationsTable(t *testing.T) { if testing.Short() { t.Skip("skipping postgres-backed test in -short mode") diff --git a/stores/blockchain/sql/SetBlockMinedSet.go b/stores/blockchain/sql/SetBlockMinedSet.go index 0ec0be2b7e..5dcf66454f 100644 --- a/stores/blockchain/sql/SetBlockMinedSet.go +++ b/stores/blockchain/sql/SetBlockMinedSet.go @@ -8,46 +8,84 @@ import ( "github.com/bsv-blockchain/teranode/errors" ) -// SetBlockMinedSet sets mined_set = true, but only while the block's invalid flag -// still equals expectInvalid, the value the caller's tagging run started from. +// SetBlockMinedSet sets mined_set = true. With a non-nil expectedGeneration the write +// only lands while the block's mined_set_gen still equals it. // -// A tagging run (BlockValidation.setTxMinedStatus) reads invalid once, tags or -// untags the block's transactions, then calls this. If InvalidateBlock or -// RevalidateBlock ran in between, they cleared mined_set so the block would be -// processed again; an unconditional write here would undo that and the block -// would never be revisited. In that case the row is left untouched and an -// ErrStateError is returned, so the caller can re-queue the block. -func (s *SQL) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectInvalid bool) error { - s.logger.Debugf("SetBlockMinedSet %s (expectInvalid=%t)", blockHash.String(), expectInvalid) +// A tagging run (BlockValidation.setTxMinedStatus) reads mined_set_gen once at the +// start, tags or untags the block's transactions, then calls this. Every write that +// clears mined_set (ClearBlockMinedSet, InvalidateBlock, RevalidateBlock) also bumps +// mined_set_gen. If one of them ran in between, an unconditional write here would undo +// the reset and the block would never be revisited. In that case the row is left +// untouched and an ErrStateError is returned, so the caller can re-queue the block. +// A counter, not the invalid flag, is compared because ClearBlockMinedSet does not +// touch invalid, and an invalidate then revalidate inside one run would put invalid +// back to its starting value. +// +// A nil expectedGeneration is the unconditional write, kept for callers that predate +// the generation (an older blockvalidation talking to this blockchain service). +func (s *SQL) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash, expectedGeneration *uint64) error { + if expectedGeneration == nil { + s.logger.Debugf("SetBlockMinedSet %s (unconditional)", blockHash.String()) + } else { + s.logger.Debugf("SetBlockMinedSet %s (expectedGeneration=%d)", blockHash.String(), *expectedGeneration) + } // Invalidate response cache to ensure cached blocks reflect updated mined_set field defer s.ResetResponseCache() - q := ` - UPDATE blocks - SET mined_set = true - WHERE hash = $1 - AND invalid = $2 - ` + var ( + res sql.Result + err error + ) + + if expectedGeneration == nil { + res, err = s.db.ExecContext(ctx, ` + UPDATE blocks + SET mined_set = true + WHERE hash = $1 + `, blockHash.CloneBytes()) + } else { + res, err = s.db.ExecContext(ctx, ` + UPDATE blocks + SET mined_set = true + WHERE hash = $1 + AND mined_set_gen = $2 + `, blockHash.CloneBytes(), *expectedGeneration) + } - res, err := s.db.ExecContext(ctx, q, blockHash.CloneBytes(), expectInvalid) if err != nil { return errors.NewStorageError("error updating block mined_set", err) } - if rows, _ := res.RowsAffected(); rows > 0 { + rows, err := res.RowsAffected() + if err != nil { + return errors.NewStorageError("error reading rows affected for block %s mined_set", blockHash.String(), err) + } + + if rows > 0 { return nil } - // Nothing updated: either the block is missing or its invalid flag moved. - var invalid bool - if err = s.db.QueryRowContext(ctx, `SELECT invalid FROM blocks WHERE hash = $1`, blockHash.CloneBytes()).Scan(&invalid); err != nil { + if expectedGeneration == nil { + return errors.NewStorageError("block %s mined_set was not updated", blockHash.String()) + } + + // Nothing updated: either the block is missing or its generation moved. + var generation uint64 + if err = s.db.QueryRowContext(ctx, `SELECT mined_set_gen FROM blocks WHERE hash = $1`, blockHash.CloneBytes()).Scan(&generation); err != nil { if errors.Is(err, sql.ErrNoRows) { return errors.NewStorageError("block %s mined_set was not updated", blockHash.String()) } - return errors.NewStorageError("error reading block %s invalid flag", blockHash.String(), err) + return errors.NewStorageError("error reading block %s mined_set_gen", blockHash.String(), err) + } + + if generation == *expectedGeneration { + // The generation did not move, so the guard was not what stopped the write. Report + // a storage fault: the caller charges retries on it, while a state error would + // re-queue for free and could loop forever. + return errors.NewStorageError("block %s mined_set was not updated although mined_set_gen is unchanged at %d", blockHash.String(), generation) } - return errors.NewStateError("block %s mined_set not updated: invalid is %t, tagging run expected %t", blockHash.String(), invalid, expectInvalid) + return errors.NewStateError("block %s mined_set not updated: mined_set_gen is %d, tagging run started at %d", blockHash.String(), generation, *expectedGeneration) } diff --git a/stores/blockchain/sql/SetBlockMinedSet_test.go b/stores/blockchain/sql/SetBlockMinedSet_test.go index 65f75b3b04..9433996d2d 100644 --- a/stores/blockchain/sql/SetBlockMinedSet_test.go +++ b/stores/blockchain/sql/SetBlockMinedSet_test.go @@ -12,7 +12,6 @@ import ( "github.com/bsv-blockchain/teranode/stores/blockchain/options" "github.com/bsv-blockchain/teranode/ulogger" "github.com/bsv-blockchain/teranode/util/test" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/modules/postgres" @@ -20,7 +19,7 @@ import ( ) // TestSQL_SetBlockMinedSet covers the conditional write on sqlite: mined_set is only set while -// the block's invalid flag still equals the value the caller's tagging run started from. +// the block's mined_set generation still equals the value the caller's tagging run started from. func TestSQL_SetBlockMinedSet(t *testing.T) { testSetBlockMinedSet(t, func(t *testing.T) *url.URL { storeURL, err := url.Parse("sqlitememory:///") @@ -31,8 +30,8 @@ func TestSQL_SetBlockMinedSet(t *testing.T) { } // TestSQL_SetBlockMinedSet_Postgres runs the same cases against real PostgreSQL, where the -// boolean bind in "invalid = $2" has to compare against a BOOLEAN column; sqlite stores it as -// an integer and cannot prove that. +// uint64 bind in "mined_set_gen = $2" has to compare against a BIGINT column and the +// ALTER TABLE migration has to add that column with its default. func TestSQL_SetBlockMinedSet_Postgres(t *testing.T) { ctx := context.Background() @@ -107,30 +106,60 @@ func testSetBlockMinedSet(t *testing.T, newStoreURL func(t *testing.T) *url.URL) return invalid, minedSet } - t.Run("sets mined_set when invalid is unchanged", func(t *testing.T) { + // generation reads the token the way a tagging run does, through GetBlockHeader + generation := func(t *testing.T, s *SQL, hash *chainhash.Hash) uint64 { + _, meta, err := s.GetBlockHeader(context.Background(), hash) + require.NoError(t, err) + + return meta.MinedSetGeneration + } + + requireRefused := func(t *testing.T, err error) { + require.Error(t, err) + require.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) + } + + t.Run("sets mined_set when the generation is unchanged", func(t *testing.T) { s := setup(t) - require.NoError(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), false)) + gen := generation(t, s, block2.Hash()) + require.NoError(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), &gen)) invalid, minedSet := flags(t, s, block2.Hash()) - assert.False(t, invalid) - assert.True(t, minedSet) + require.False(t, invalid) + require.True(t, minedSet) + }) + + t.Run("every writer that clears mined_set bumps the generation", func(t *testing.T) { + s := setup(t) + + start := generation(t, s, block2.Hash()) + + require.NoError(t, s.ClearBlockMinedSet(context.Background(), block2.Hash())) + require.Equal(t, start+1, generation(t, s, block2.Hash()), "ClearBlockMinedSet") + + _, err := s.InvalidateBlock(context.Background(), block2.Hash()) + require.NoError(t, err) + require.Equal(t, start+2, generation(t, s, block2.Hash()), "InvalidateBlock") + + require.NoError(t, s.RevalidateBlock(context.Background(), block2.Hash())) + require.Equal(t, start+3, generation(t, s, block2.Hash()), "RevalidateBlock") }) t.Run("refuses after the block was invalidated mid-run", func(t *testing.T) { s := setup(t) // a tagging run started while the block was valid, then the block was invalidated + gen := generation(t, s, block2.Hash()) + _, err := s.InvalidateBlock(context.Background(), block2.Hash()) require.NoError(t, err) - err = s.SetBlockMinedSet(context.Background(), block2.Hash(), false) - require.Error(t, err) - assert.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) + requireRefused(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), &gen)) invalid, minedSet := flags(t, s, block2.Hash()) - assert.True(t, invalid) - assert.False(t, minedSet, "the invalidation's mined_set=false must survive, so the block is processed again") + require.True(t, invalid) + require.False(t, minedSet, "the invalidation's mined_set=false must survive, so the block is processed again") }) t.Run("refuses after the block was revalidated mid-run", func(t *testing.T) { @@ -140,15 +169,47 @@ func testSetBlockMinedSet(t *testing.T, newStoreURL func(t *testing.T) *url.URL) _, err := s.InvalidateBlock(context.Background(), block2.Hash()) require.NoError(t, err) + gen := generation(t, s, block2.Hash()) + require.NoError(t, s.RevalidateBlock(context.Background(), block2.Hash())) - err = s.SetBlockMinedSet(context.Background(), block2.Hash(), true) - require.Error(t, err) - assert.True(t, errors.Is(err, errors.ErrStateError), "expected ErrStateError, got %v", err) + requireRefused(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), &gen)) + + invalid, minedSet := flags(t, s, block2.Hash()) + require.False(t, invalid) + require.False(t, minedSet, "the revalidation's mined_set=false must survive, so the block is tagged again") + }) + + t.Run("refuses after ClearBlockMinedSet mid-run, which leaves invalid untouched", func(t *testing.T) { + s := setup(t) + + // catchup clears mined_set on a block that moved off the main chain; a run that + // started before, with onLongestChain=true, must not write mined_set back + gen := generation(t, s, block2.Hash()) + + require.NoError(t, s.ClearBlockMinedSet(context.Background(), block2.Hash())) + + requireRefused(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), &gen)) + + invalid, minedSet := flags(t, s, block2.Hash()) + require.False(t, invalid, "precondition: the clear did not move invalid, so a guard on invalid would have let the write through") + require.False(t, minedSet, "the clear's mined_set=false must survive, so the block is processed again") + }) + + t.Run("refuses after an invalidate and revalidate mid-run put invalid back", func(t *testing.T) { + s := setup(t) + + gen := generation(t, s, block2.Hash()) + + _, err := s.InvalidateBlock(context.Background(), block2.Hash()) + require.NoError(t, err) + require.NoError(t, s.RevalidateBlock(context.Background(), block2.Hash())) + + requireRefused(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), &gen)) invalid, minedSet := flags(t, s, block2.Hash()) - assert.False(t, invalid) - assert.False(t, minedSet, "the revalidation's mined_set=false must survive, so the block is tagged again") + require.False(t, invalid, "precondition: invalid is back at its starting value") + require.False(t, minedSet, "the stale run must not commit over the revalidation's reset") }) t.Run("sets mined_set on an invalid block for an untagging run", func(t *testing.T) { @@ -157,19 +218,72 @@ func testSetBlockMinedSet(t *testing.T, newStoreURL func(t *testing.T) *url.URL) _, err := s.InvalidateBlock(context.Background(), block2.Hash()) require.NoError(t, err) - require.NoError(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), true)) + gen := generation(t, s, block2.Hash()) + require.NoError(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), &gen)) invalid, minedSet := flags(t, s, block2.Hash()) - assert.True(t, invalid) - assert.True(t, minedSet) + require.True(t, invalid) + require.True(t, minedSet) + }) + + t.Run("nil generation is the unconditional write an older caller expects", func(t *testing.T) { + s := setup(t) + + _, err := s.InvalidateBlock(context.Background(), block2.Hash()) + require.NoError(t, err) + + require.NoError(t, s.SetBlockMinedSet(context.Background(), block2.Hash(), nil)) + + _, minedSet := flags(t, s, block2.Hash()) + require.True(t, minedSet) }) t.Run("unknown block is a storage error, not a state error", func(t *testing.T) { s := setup(t) - err := s.SetBlockMinedSet(context.Background(), block3.Hash(), false) + gen := uint64(0) + err := s.SetBlockMinedSet(context.Background(), block3.Hash(), &gen) + require.Error(t, err) + require.True(t, errors.Is(err, errors.ErrStorageError), "expected ErrStorageError, got %v", err) + require.False(t, errors.Is(err, errors.ErrStateError)) + + err = s.SetBlockMinedSet(context.Background(), block3.Hash(), nil) require.Error(t, err) - assert.True(t, errors.Is(err, errors.ErrStorageError), "expected ErrStorageError, got %v", err) - assert.False(t, errors.Is(err, errors.ErrStateError)) + require.True(t, errors.Is(err, errors.ErrStorageError), "expected ErrStorageError, got %v", err) }) } + +// TestSQL_SetBlockMinedSet_UnchangedGenerationIsStorageError covers the zero-rows path where the +// generation did NOT move. The guard was not what stopped the write, so it must be a storage +// error: the caller charges retries on that, while a state error re-queues for free and would +// loop forever on a real storage fault. A sqlite trigger that drops the UPDATE stands in for +// that fault. +func TestSQL_SetBlockMinedSet_UnchangedGenerationIsStorageError(t *testing.T) { + storeURL, err := url.Parse("sqlitememory:///") + require.NoError(t, err) + + s, err := New(ulogger.TestLogger{}, storeURL, test.CreateBaseTestSettings(t)) + require.NoError(t, err) + + defer func() { _ = s.Close(context.Background()) }() + + _, _, err = s.StoreBlock(context.Background(), block1, "", options.WithMinedSet(true)) + require.NoError(t, err) + + _, _, err = s.StoreBlock(context.Background(), block2, "") + require.NoError(t, err) + + _, meta, err := s.GetBlockHeader(context.Background(), block2.Hash()) + require.NoError(t, err) + + gen := meta.MinedSetGeneration + + _, err = s.db.ExecContext(context.Background(), + `CREATE TRIGGER drop_mined_set_update BEFORE UPDATE OF mined_set ON blocks BEGIN SELECT RAISE(IGNORE); END;`) + require.NoError(t, err) + + err = s.SetBlockMinedSet(context.Background(), block2.Hash(), &gen) + require.Error(t, err) + require.True(t, errors.Is(err, errors.ErrStorageError), "expected ErrStorageError, got %v", err) + require.False(t, errors.Is(err, errors.ErrStateError), "an unchanged generation must not read as a refused write") +} diff --git a/stores/blockchain/sql/sql.go b/stores/blockchain/sql/sql.go index 5fc4c05f72..aa52ef6176 100644 --- a/stores/blockchain/sql/sql.go +++ b/stores/blockchain/sql/sql.go @@ -523,6 +523,7 @@ var blockchainSchemaExpectedColumns = []string{ "median_time_past", "coinbase_bump", "on_main_chain", + "mined_set_gen", } // blockchainSchemaExpectedIndexes is the list of indexes on blocks that the @@ -671,6 +672,7 @@ func createPostgresSchemaUnlocked(db *usql.DB, withIndexes bool) error { ,median_time_past BIGINT NOT NULL DEFAULT 0 ,coinbase_bump BYTEA NULL ,on_main_chain BOOLEAN NOT NULL DEFAULT FALSE + ,mined_set_gen BIGINT NOT NULL DEFAULT 0 ); `); err != nil { _ = db.Close() @@ -767,6 +769,23 @@ func createPostgresSchemaUnlocked(db *usql.DB, withIndexes bool) error { } } + // add the mined_set_gen column to the blocks table if it does not exist. Every write that + // clears mined_set bumps it, so SetBlockMinedSet can refuse a tagging run that started + // before the clear (see SetBlockMinedSet.go). + err = db.QueryRow("SELECT column_name FROM information_schema.columns WHERE table_name='blocks' AND column_name='mined_set_gen'").Scan(new(string)) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + _, err := db.Exec(`ALTER TABLE blocks ADD COLUMN mined_set_gen BIGINT NOT NULL DEFAULT 0;`) + if err != nil { + _ = db.Close() + return errors.NewStorageError("could not add mined_set_gen column to blocks table", err) + } + } else { + _ = db.Close() + return errors.NewStorageError("could not check for mined_set_gen column in blocks table", err) + } + } + if _, err := db.Exec(`CREATE UNIQUE INDEX IF NOT EXISTS ux_blocks_hash ON blocks (hash);`); err != nil { _ = db.Close() return errors.NewStorageError("could not create ux_blocks_hash index", err) @@ -988,6 +1007,7 @@ func createSqliteSchema(db *usql.DB) error { ,median_time_past BIGINT NOT NULL DEFAULT 0 ,coinbase_bump BLOB NULL ,on_main_chain BOOLEAN NOT NULL DEFAULT FALSE + ,mined_set_gen BIGINT NOT NULL DEFAULT 0 ); `); err != nil { _ = db.Close() @@ -1079,6 +1099,23 @@ func createSqliteSchema(db *usql.DB) error { } } + // add the mined_set_gen column to the blocks table if it does not exist. Every write that + // clears mined_set bumps it, so SetBlockMinedSet can refuse a tagging run that started + // before the clear (see SetBlockMinedSet.go). + err = db.QueryRow("SELECT name FROM pragma_table_info('blocks') WHERE name='mined_set_gen'").Scan(new(string)) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + _, err := db.Exec(`ALTER TABLE blocks ADD COLUMN mined_set_gen BIGINT NOT NULL DEFAULT 0;`) + if err != nil { + _ = db.Close() + return errors.NewStorageError("could not add mined_set_gen column to blocks table", err) + } + } else { + _ = db.Close() + return errors.NewStorageError("could not check for mined_set_gen column in blocks table", err) + } + } + if _, err := db.Exec(`CREATE UNIQUE INDEX IF NOT EXISTS ux_blocks_hash ON blocks (hash);`); err != nil { _ = db.Close() return errors.NewStorageError("could not create ux_blocks_hash index", err) From da5cf29fd5c685caf33ded42d75b926f6e63f324 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 28 Oct 2026 12:35:19 +0000 Subject: [PATCH 4/5] fix(blockvalidation): a refused mined_set write re-queues after a backoff and keeps the retry counter When the mined_set generation moved under a tagging run, setTxMinedStatus put the block straight back on setMinedChan and returned nil. The worker then took nil as success and deleted the block's retry counter, so a block that had used 9 of its 10 attempts on real storage errors started again from 0 after one refusal. The re-queue also had no wait, so a generation that kept moving drove back-to-back full re-tags. setTxMinedStatus now returns a dedicated re-queue error for both re-queue cases (the refused write and the stale invalid flag). The worker leaves the retry counter as it is, neither charged nor reset, and re-queues through the existing backoff path, at least 1 s. The CHANGELOG entry and the ClearBlockMinedSet test comment no longer imply the catchup fork case is closed. Catchup clears mined_set before the reorg, so the re-queued run still tags the block as on the main chain. That ordering predates this branch. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016HzSYZUnpxNSz4kXw4T5yR --- CHANGELOG.md | 8 +- .../block_validation_set_tx_mined.puml | 2 +- services/blockvalidation/BlockValidation.go | 49 +++++++++--- .../blockvalidation/BlockValidation_test.go | 76 +++++++++++++++++++ services/blockvalidation/gap_bv058_test.go | 43 ++++++----- 5 files changed, 145 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdf89aaff3..4d87578033 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,8 +113,12 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea `GetBlockHeaderResponse` gains `mined_set_generation`. When the request carries the field, the write only lands if the generation has not moved since the caller read it. Otherwise the call returns `ERR_STATE_ERROR`, and block validation re-queues the - block. This stops a set-mined run that was in flight during an invalidation or a - catchup reset from overwriting that reset. Mixed versions keep working, so there is + block after a backoff of at least 1 s, without charging or resetting its retry + counter. This stops a set-mined run that was in flight during an invalidation or a + catchup reset from overwriting that reset. It does not close the catchup fork case + on its own: catchup clears `mined_set` on the old-chain blocks before the reorg, so + the re-queued run still finds the block on the main chain and tags it again. That + ordering predates this change. Mixed versions keep working, so there is no required upgrade order. An older block validation service sends no generation, and a new blockchain service writes for it unconditionally, as before. A new block validation service talking to an older blockchain service has its field ignored, and diff --git a/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml b/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml index 8cf03e7da1..e2937fe7c1 100644 --- a/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml +++ b/docs/topics/services/img/plantuml/blockvalidation/block_validation_set_tx_mined.puml @@ -128,7 +128,7 @@ alt Generation unchanged Blockchain --> BlockValidation: ok else Generation moved during the run Blockchain --> BlockValidation: ERR_STATE_ERROR - BlockValidation -> BVServer: Re-queue on setMinedChan (retry counter not charged) + BlockValidation -> BVServer: Re-queue on setMinedChan after a backoff (retry counter unchanged) end deactivate Blockchain diff --git a/services/blockvalidation/BlockValidation.go b/services/blockvalidation/BlockValidation.go index 083d2216ef..9a97f7ce0b 100644 --- a/services/blockvalidation/BlockValidation.go +++ b/services/blockvalidation/BlockValidation.go @@ -128,6 +128,20 @@ const ( setMinedMaxBackoff = 16 * time.Second ) +// setMinedRequeueError is what setTxMinedStatus returns when the block's state moved +// under the run: its final mined_set write was refused because the generation moved, or +// the invalid flag changed before the run started. The run did not fail, so the setMined +// worker neither charges nor resets the block's retry counter. It re-queues the block +// through the backoff path, so a generation that keeps moving cannot drive back-to-back +// full re-tags of a large block. +type setMinedRequeueError struct { + reason string +} + +func (e *setMinedRequeueError) Error() string { + return "set-mined run must be re-queued: " + e.reason +} + // setMinedRetryBackoff computes the exponential-with-cap backoff for the // (attempt)th retry. attempt is 1-indexed (1 = first retry after the initial // failure). 1s, 2s, 4s, 8s, 16s, 16s, ... @@ -800,6 +814,26 @@ func (u *BlockValidation) start(ctx context.Context) error { default: } + var requeue *setMinedRequeueError + if errors.As(err, &requeue) { + // The block's state moved under the run; it did not fail. Leave the + // retry counter as it is, neither charged nor reset, so a block that + // is also failing for real still reaches setMinedMaxRetries. Wait at + // the backoff for the attempts already made (1s at least) rather than + // re-queueing at once, so repeated resets cannot drive back-to-back + // full re-tags. + var attempt uint64 + if prev, ok := u.setMinedRetries.Load(*blockHash); ok { + attempt = prev.(uint64) + } + + backoff := setMinedRetryBackoff(int(attempt)) + u.logger.Warnf("[BlockValidation:start][%s] setTxMined re-queued in %s, retry counter unchanged at %d: %s", blockHash.String(), backoff, attempt, err) + u.scheduleSetMinedRetry(ctx, blockHash, backoff) + + return + } + u.logger.Errorf("[BlockValidation:start][%s] failed setTxMined: %s", blockHash.String(), err) if errors.Is(err, errors.ErrBlockNotFound) { @@ -1573,8 +1607,8 @@ func (u *BlockValidation) setTxMinedStatus(ctx context.Context, blockHash *chain // The caller read the invalid flag earlier, in its own GetBlockHeader. If an invalidate // or revalidate landed between that read and this one, the generation above already - // counts it, so the guard cannot catch a run started on the stale flag. Re-queue - // instead; the next pick-up reads both together. + // counts it, so the guard cannot catch a run started on the stale flag. Ask the + // worker to re-queue instead; the next pick-up reads both together. if startedInvalid := len(unsetMined) > 0 && unsetMined[0]; startedInvalid != blockHeaderMeta.Invalid { u.logger.Warnf("[setTxMined][%s] block invalid flag changed before the run started (caller saw %t, now %t), re-queueing", blockHash.String(), startedInvalid, blockHeaderMeta.Invalid) @@ -1584,9 +1618,7 @@ func (u *BlockValidation) setTxMinedStatus(ctx context.Context, blockHash *chain u.logger.Warnf("[setTxMined][%s] failed closing subtrees before re-queue: %v", blockHash.String(), releaseErr) } - u.enqueueSetMined(blockHash) - - return nil + return &setMinedRequeueError{reason: fmt.Sprintf("invalid flag changed before the run started (caller saw %t, now %t)", startedInvalid, blockHeaderMeta.Invalid)} } onLongestChain, err = u.blockchainClient.CheckBlockIsInCurrentChain(ctx, []uint32{blockHeaderMeta.ID}) @@ -1695,12 +1727,11 @@ func (u *BlockValidation) setTxMinedStatus(ctx context.Context, blockHash *chain // longer holds. if err = u.blockchainClient.SetBlockMinedSet(ctx, blockHash, minedSetGeneration); err != nil { if errors.Is(err, errors.ErrStateError) { - // Not a failure of this run: re-queue so the block is processed again under its - // new state, and return nil so the retry counter is not charged. + // Not a failure of this run: the block must be processed again under its new + // state. The worker re-queues it after a backoff and leaves the retry counter alone. u.logger.Warnf("[setTxMined][%s] block mined_set was cleared during the run, re-queueing: %v", blockHash.String(), err) - u.enqueueSetMined(blockHash) - return nil + return &setMinedRequeueError{reason: "mined_set generation moved during the run: " + err.Error()} } return errors.NewServiceError("[setTxMined][%s] failed to set block mined", block.Hash().String(), err) diff --git a/services/blockvalidation/BlockValidation_test.go b/services/blockvalidation/BlockValidation_test.go index 6c594cbf77..7496044d1b 100644 --- a/services/blockvalidation/BlockValidation_test.go +++ b/services/blockvalidation/BlockValidation_test.go @@ -4744,6 +4744,82 @@ func TestSetMinedChanWorker_RetryPendingDedup(t *testing.T) { require.Equal(t, uint64(1), attempts, "skipped re-triggers must not inflate the retry counter") } +// setMinedRequeueClient is a decorator over a real blockchain.ClientI that drives every run +// for one hash into setTxMinedStatus's re-queue path. The worker's GetBlockHeader (odd calls) +// reports invalid=false; the run's own GetBlockHeader (even calls) reports invalid=true, so the +// run sees the flag move before it started and asks the worker to re-queue. +type setMinedRequeueClient struct { + blockchain.ClientI + + hash *chainhash.Hash + headerCalls atomic.Int32 +} + +func (c *setMinedRequeueClient) GetBlockHeader(ctx context.Context, hash *chainhash.Hash) (*model.BlockHeader, *model.BlockHeaderMeta, error) { + if hash.IsEqual(c.hash) { + n := c.headerCalls.Add(1) + return &model.BlockHeader{}, &model.BlockHeaderMeta{ID: 1, Invalid: n%2 == 0}, nil + } + + return c.ClientI.GetBlockHeader(ctx, hash) +} + +func (c *setMinedRequeueClient) GetBlock(ctx context.Context, hash *chainhash.Hash) (*model.Block, error) { + if hash.IsEqual(c.hash) { + return &model.Block{Header: &model.BlockHeader{}}, nil + } + + return c.ClientI.GetBlock(ctx, hash) +} + +// TestSetMinedChanWorker_RequeueKeepsRetryCounterAndBacksOff covers a run whose block state moved +// under it. That run did not fail, so the worker must leave the retry counter exactly where it was: +// resetting it would let one refusal wipe out the attempts a block with real storage errors has +// already used, and charging it would drop a block whose state is merely busy. The re-queue must go +// through the backoff path, not straight back on setMinedChan, so a generation that keeps moving +// cannot drive back-to-back full re-tags. +func TestSetMinedChanWorker_RequeueKeepsRetryCounterAndBacksOff(t *testing.T) { + initPrometheusMetrics() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + utxoStore, subtreeValidationClient, blockchainClient, txStore, subtreeStore, deferFunc := setup(t) + defer deferFunc() + + tSettings := test.CreateBaseTestSettings(t) + + h := chainhash.HashH([]byte("requeue-hash")) + client := &setMinedRequeueClient{ClientI: blockchainClient, hash: &h} + + blockValidation := NewBlockValidation(ctx, ulogger.TestLogger{}, tSettings, client, subtreeStore, txStore, utxoStore, nil, subtreeValidationClient) + + // The block has already used one charged attempt for a real failure. + blockValidation.setMinedRetries.Store(h, uint64(1)) + + blockValidation.setMinedChan <- &h + + require.Eventually(t, func() bool { + _, pending := blockValidation.setMinedRetryPending.Load(h) + return pending + }, 2*time.Second, 5*time.Millisecond, "the re-queue must go through the backoff path") + + require.Equal(t, int32(2), client.headerCalls.Load(), "exactly one run so far: the re-queue must wait out its backoff") + + attempts, ok := blockValidation.setMinedRetries.Load(h) + require.True(t, ok, "a re-queue must not reset the retry counter") + require.Equal(t, uint64(1), attempts, "a re-queue must not charge the retry counter") + + // After the 1s backoff the block is picked up again, and the counter is still untouched. + require.Eventually(t, func() bool { + return client.headerCalls.Load() >= 4 + }, 3*time.Second, 20*time.Millisecond, "the re-queued block was never processed again") + + attempts, ok = blockValidation.setMinedRetries.Load(h) + require.True(t, ok) + require.Equal(t, uint64(1), attempts, "repeated re-queues must leave the retry counter alone") +} + // TestIsParentFinalizing guards the child parent-wait fix: checkParentProcessingComplete // must treat a parent that is either actively being processed OR waiting out a setTxMined // retry backoff as "still finalizing", so a child keeps waiting across the parent's whole diff --git a/services/blockvalidation/gap_bv058_test.go b/services/blockvalidation/gap_bv058_test.go index e6f28308db..cce35d939f 100644 --- a/services/blockvalidation/gap_bv058_test.go +++ b/services/blockvalidation/gap_bv058_test.go @@ -233,16 +233,15 @@ func (f *gap058Fixture) runParkedTagging(t *testing.T, interfere func()) error { return nil } -// requireRequeued asserts the refused run put the block back on setMinedChan. -func (f *gap058Fixture) requireRequeued(t *testing.T) { +// requireRequeued asserts the refused run asked the setMined worker to re-queue the block. The +// run itself must not put the block straight back on setMinedChan: the worker re-queues it +// through the backoff path, which TestSetMinedChanWorker_RequeueKeepsRetryCounterAndBacksOff covers. +func (f *gap058Fixture) requireRequeued(t *testing.T, runErr error) { t.Helper() - select { - case requeued := <-f.bv.setMinedChan: - require.Equal(t, *f.block.Hash(), *requeued, "the refused run must re-queue the block") - default: - t.Fatal("the refused run did not re-queue the block on setMinedChan") - } + var requeue *setMinedRequeueError + require.ErrorAs(t, runErr, &requeue, "the refused run must return setMinedRequeueError, not nil and not a plain failure") + require.Len(t, f.bv.setMinedChan, 0, "the run must leave the re-queue, and its backoff, to the worker") } // TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation is the regression test for GAP-BV-058 @@ -253,7 +252,8 @@ func (f *gap058Fixture) requireRequeued(t *testing.T) { // the block is processed again; an unconditional SetBlockMinedSet at the end of the run used to // overwrite that, after which the setMined worker's MinedSet guard and the mined-not-set sweep both // skipped the block for good. SetBlockMinedSet is now conditional on the mined_set generation the run -// started from, and setTxMinedStatus re-queues the block when the write is refused. The race is made +// started from, and setTxMinedStatus asks the worker to re-queue the block when the write is +// refused. The race is made // deterministic with a UTXO-store decorator that parks the tagging run inside SetMinedMulti while // InvalidateBlock runs on the real sqlitememory store. func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { @@ -270,10 +270,6 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { require.False(t, midMeta.MinedSet, "precondition: InvalidateBlock cleared mined_set") }) - // nil, not the state error: a refused write is not a failed run and must not - // charge the setMined retry counter - require.NoError(t, runErr, "the refused run must return nil") - _, endMeta, err := f.blockchainClient.GetBlockHeader(ctx, f.block.Hash()) require.NoError(t, err) require.True(t, endMeta.Invalid, "the block is still invalid at the end") @@ -285,7 +281,9 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { require.False(t, endMeta.MinedSet, "the refused write must leave mined_set=false so the sweep still lists the block") - f.requireRequeued(t) + // a refused write is not a failed run: the run returns the re-queue error, which the + // worker handles without charging the retry counter + f.requireRequeued(t, runErr) require.False(t, endMeta.MinedSet && tagsPresent, "GAP-BV-058: block is invalid, yet the in-flight run left mined_set=true with the transaction still tagged (BlockIDs=%v); the unconditional SetBlockMinedSet overwrote the invalidation and the sweep will never revisit it", endTags.BlockIDs) @@ -294,8 +292,13 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteInvalidation(t *testing.T) { // TestGapBV058_InFlightTaggingDoesNotOverwriteClearBlockMinedSet covers the reset that does not // touch the invalid flag. Catchup calls ClearBlockMinedSet on a block that moved off the main chain. // A run that started before, with onLongestChain=true, used to match a guard on invalid and write -// mined_set=true over the clear, after which the re-queued run skipped on MinedSet=true and the -// block's transactions stayed tagged as on the longest chain. The generation guard refuses it. +// mined_set=true over the clear, after which the re-queued run skipped on MinedSet=true. The +// generation guard refuses that write, which is all this test proves. +// +// It does not prove the catchup fork case is closed. Catchup clears mined_set on the old-chain +// blocks before the reorg (catchup.go, step 5.5), so the re-queued run still finds the block on the +// main chain, tags it with onLongestChain=true and sets mined_set=true, and nothing clears it again +// after the real reorg. That ordering predates this change and needs a clear after the reorg. func TestGapBV058_InFlightTaggingDoesNotOverwriteClearBlockMinedSet(t *testing.T) { f := newGap058Fixture(t) ctx := f.ctx @@ -304,14 +307,12 @@ func TestGapBV058_InFlightTaggingDoesNotOverwriteClearBlockMinedSet(t *testing.T require.NoError(t, f.blockchainClient.ClearBlockMinedSet(ctx, f.block.Hash())) }) - require.NoError(t, runErr, "the refused run must return nil") - _, endMeta, err := f.blockchainClient.GetBlockHeader(ctx, f.block.Hash()) require.NoError(t, err) require.False(t, endMeta.Invalid, "precondition: the clear left invalid untouched") require.False(t, endMeta.MinedSet, "the run must not write mined_set=true over ClearBlockMinedSet") - f.requireRequeued(t) + f.requireRequeued(t, runErr) } // TestGapBV058_InvalidFlagMovedBeforeRunStarts covers the window between the worker's read of @@ -326,7 +327,7 @@ func TestGapBV058_InvalidFlagMovedBeforeRunStarts(t *testing.T) { _, err := f.blockchainClient.InvalidateBlock(ctx, f.block.Hash()) require.NoError(t, err) - require.NoError(t, f.bv.setTxMinedStatus(ctx, f.block.Hash(), false)) + runErr := f.bv.setTxMinedStatus(ctx, f.block.Hash(), false) _, endMeta, err := f.blockchainClient.GetBlockHeader(ctx, f.block.Hash()) require.NoError(t, err) @@ -337,5 +338,5 @@ func TestGapBV058_InvalidFlagMovedBeforeRunStarts(t *testing.T) { require.NoError(t, err) require.Empty(t, endTags.BlockIDs, "a run started on the stale flag must not tag the invalid block's transaction") - f.requireRequeued(t) + f.requireRequeued(t, runErr) } From cbfd4d5678684b385d61d577f045cec81c17ae7c Mon Sep 17 00:00:00 2001 From: freemans13 Date: Thu, 29 Oct 2026 12:15:02 +0000 Subject: [PATCH 5/5] refactor(blockchain): share the mined_set_gen column migration between engines The postgres and sqlite schema builders each carried the same 14-line probe-then-ALTER block for mined_set_gen. SonarQube counted it as 34 of the 37 new lines in sql.go and failed the PR on duplication (3.3% against a 3% limit). Both now call addMinedSetGenColumn with their engine's probe query. Behaviour is unchanged: the postgres upgrade test still drops the column, reopens the store and sees it added back with existing rows at 0. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016HzSYWhzVWctuAFYQTLyXn --- stores/blockchain/sql/sql.go | 50 +++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/stores/blockchain/sql/sql.go b/stores/blockchain/sql/sql.go index aa52ef6176..f39dff4326 100644 --- a/stores/blockchain/sql/sql.go +++ b/stores/blockchain/sql/sql.go @@ -772,18 +772,8 @@ func createPostgresSchemaUnlocked(db *usql.DB, withIndexes bool) error { // add the mined_set_gen column to the blocks table if it does not exist. Every write that // clears mined_set bumps it, so SetBlockMinedSet can refuse a tagging run that started // before the clear (see SetBlockMinedSet.go). - err = db.QueryRow("SELECT column_name FROM information_schema.columns WHERE table_name='blocks' AND column_name='mined_set_gen'").Scan(new(string)) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - _, err := db.Exec(`ALTER TABLE blocks ADD COLUMN mined_set_gen BIGINT NOT NULL DEFAULT 0;`) - if err != nil { - _ = db.Close() - return errors.NewStorageError("could not add mined_set_gen column to blocks table", err) - } - } else { - _ = db.Close() - return errors.NewStorageError("could not check for mined_set_gen column in blocks table", err) - } + if err := addMinedSetGenColumn(db, "SELECT column_name FROM information_schema.columns WHERE table_name='blocks' AND column_name='mined_set_gen'"); err != nil { + return err } if _, err := db.Exec(`CREATE UNIQUE INDEX IF NOT EXISTS ux_blocks_hash ON blocks (hash);`); err != nil { @@ -966,6 +956,28 @@ func createPostgresSchemaUnlocked(db *usql.DB, withIndexes bool) error { return nil } +// addMinedSetGenColumn adds the mined_set_gen column to blocks when probeQuery, which must +// select one row naming the column if it exists, finds nothing. It closes db on failure, as +// the schema builders that call it do on every other error. +func addMinedSetGenColumn(db *usql.DB, probeQuery string) error { + err := db.QueryRow(probeQuery).Scan(new(string)) + if err == nil { + return nil + } + + if !errors.Is(err, sql.ErrNoRows) { + _ = db.Close() + return errors.NewStorageError("could not check for mined_set_gen column in blocks table", err) + } + + if _, err = db.Exec(`ALTER TABLE blocks ADD COLUMN mined_set_gen BIGINT NOT NULL DEFAULT 0;`); err != nil { + _ = db.Close() + return errors.NewStorageError("could not add mined_set_gen column to blocks table", err) + } + + return nil +} + func createSqliteSchema(db *usql.DB) error { if _, err := db.Exec(` CREATE TABLE IF NOT EXISTS state ( @@ -1102,18 +1114,8 @@ func createSqliteSchema(db *usql.DB) error { // add the mined_set_gen column to the blocks table if it does not exist. Every write that // clears mined_set bumps it, so SetBlockMinedSet can refuse a tagging run that started // before the clear (see SetBlockMinedSet.go). - err = db.QueryRow("SELECT name FROM pragma_table_info('blocks') WHERE name='mined_set_gen'").Scan(new(string)) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - _, err := db.Exec(`ALTER TABLE blocks ADD COLUMN mined_set_gen BIGINT NOT NULL DEFAULT 0;`) - if err != nil { - _ = db.Close() - return errors.NewStorageError("could not add mined_set_gen column to blocks table", err) - } - } else { - _ = db.Close() - return errors.NewStorageError("could not check for mined_set_gen column in blocks table", err) - } + if err := addMinedSetGenColumn(db, "SELECT name FROM pragma_table_info('blocks') WHERE name='mined_set_gen'"); err != nil { + return err } if _, err := db.Exec(`CREATE UNIQUE INDEX IF NOT EXISTS ux_blocks_hash ON blocks (hash);`); err != nil {