feat(netsync): pre-assign block ID in legacy path to enable skipping setTxMined#711
Merged
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: No critical issues found. The implementation correctly optimizes UTXO creation during LEGACYSYNCING by:
Documentation claims verified:
Addressed human reviewer question about parameter design in inline comment thread. |
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-04-17 16:23 UTC |
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: [Major] Protobuf documentation not regenerated The
Code Quality: The implementation is well-structured and follows the existing patterns in the codebase: ✅ Good practices observed:
✅ Logic correctness:
Summary: One documentation issue found (protobuf docs not regenerated). Code implementation is solid and well-documented. No logic errors or security concerns identified. |
freemans13
approved these changes
Apr 16, 2026
…setTxMined In LEGACYSYNCING mode, the legacy netsync path (handle_block.go) now: 1. Calls GetNextBlockID before creating UTXOs, so every UTXO is stored with the correct BlockID/BlockHeight via WithMinedBlockInfo from the start. 2. Threads the pre-assigned block ID through ProcessBlock → blockvalidation gRPC (new ProcessBlockRequest.block_id proto field). 3. blockvalidation Server sets block.ID from the request, and ValidateBlock calls AddBlock(WithID, WithMinedSet=true) when block.ID is pre-assigned. This causes the existing setMinedChan worker guard (BlockValidation.go:533-536) to see mined_set=true and skip setTxMinedStatus, eliminating the redundant SetMinedMulti round-trip over every UTXO for legacy-synced blocks.
…pend bypass and ID gap tolerance
7e0858a to
a4e0931
Compare
freemans13
requested changes
Apr 17, 2026
| } | ||
|
|
||
| func (mv *MockBlockValidation) ProcessBlock(ctx context.Context, block *model.Block, blockHeight uint32, peerID, baseURL string) error { | ||
| func (mv *MockBlockValidation) ProcessBlock(ctx context.Context, block *model.Block, blockHeight uint32, peerID, baseURL string, blockID uint32) error { |
Collaborator
There was a problem hiding this comment.
model.Block has ID attribute. I'm wondering whether we should be populating model.Block.ID rather than adding blockID parameter to each func? You can argue same for blockHeight param vs existing model.Block.Height attribute
Contributor
There was a problem hiding this comment.
Good observation. The PR does populate model.Block.ID (at handle_block.go:166), and that's where the block ID is initially set.
The separate blockID parameter exists because block.Bytes() (the wire serialization) does not include the ID field — it's an internal database field, not part of the Bitcoin wire protocol. When the block crosses the gRPC boundary:
Client.ProcessBlockcallsblock.Bytes()→ ID is lost in serialization- The proto field
BlockIdcarries it separately (Client.go:164) Server.ProcessBlockrestores it:block.ID = request.BlockId(Server.go:1268)
Reading teranodeBlock.ID in the netsync call (handle_block.go:246) avoids duplication as a separate variable, but the proto field is necessary to preserve the ID across the service boundary.
teranodeBlock.ID is populated by model.NewBlock at construction time, so sm.ProcessBlock can read it from the struct instead of accepting it as a separate parameter. The proto field on the gRPC boundary has to stay because block.Bytes() does not serialize ID.
|
freemans13
approved these changes
Apr 17, 2026
oskarszoon
approved these changes
Apr 22, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Approve. Clean replication of the catchup path's block-ID-preassignment pattern for the legacy netsync path. Proto field backward-compatible, all call sites updated, CI green.
Minor suggestions (non-blocking):
- Consider adding a unit test for the happy path where GetNextBlockID returns an ID and it propagates through to utxoStore.Create with WithMinedBlockInfo
- buildAddBlockOpts doc comment could be trimmed to ~6 lines (keep the trade-off rationale, drop the restated mechanics)
icellan
added a commit
that referenced
this pull request
Apr 27, 2026
Adapts new double-spend test files to the 6-arg ProcessBlock signature introduced by #711 ("pre-assign block ID in legacy path"). All callers in the legacy path use blockID=0, matching every existing test caller.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
During LEGACYSYNCING, the legacy netsync path (
handle_block.go) callscreateUtxoswhich creates UTXOs without a block ID. Later, blockvalidation runs fullValidateBlockWithOptions, adds the block to the blockchain (auto-assigning an ID), thenupdateSubtreesDAHtriggers thesetMinedChanbackground worker. That worker callssetTxMinedStatus→UpdateTxMinedStatus→SetMinedMultito retroactively write the block ID to every UTXO in the block. This is a redundant round-trip for each transaction in every legacy-synced block.The catchup path (
quickValidateBlock) already avoids this by:GetNextBlockIDupfront to get block ID XWithMinedBlockInfo{BlockID: X}AddBlock(WithID(X), WithMinedSet(true))— block stored withmined_set = truesetMinedChanworker seesblockHeaderMeta.MinedSet == true(guard atBlockValidation.go:533–536) and skipssetTxMinedStatusentirelyThis PR replicates the same pattern for the legacy netsync path.
Solution
Three layers of change:
1. Netsync (
handle_block.go)prepareSubtreesnow callsblockchainClient.GetNextBlockIDonly when in LEGACYSYNCING mode (quickValidationMode = legacyMode) and returns the assigned block IDValidateTransactionsLegacyMode→createUtxos, which now passesWithMinedBlockInfo{BlockID, BlockHeight, SubtreeIdx: 0}toutxoStore.CreateProcessBlockaccepts the block ID and forwards it to blockvalidation2. gRPC interface (
blockvalidation_api.proto)block_id uint32field added toProcessBlockRequest(field 5)3. Blockvalidation (
Server.go+BlockValidation.go)Server.ProcessBlocksetsblock.ID = request.BlockIdwhen non-zerobuildAddBlockOptshelper: whenblock.ID != 0, returns[WithID(block.ID), WithMinedSet(true)]AddBlockcall sites inValidateBlockWithOptionsuse this helper, so blocks arriving from the legacy netsync path are stored withmined_set = trueupfrontsetMinedChanguard (blockHeaderMeta.MinedSet == true→continue) then skipssetTxMinedStatusentirely for those blocksWhy the gRPC field is necessary
block.Bytes()(the wire serialisation used in the gRPC request) does not includeblock.ID— it is an internal database field. Without the new proto field, the pre-assigned ID is lost at the service boundary and blockvalidation would auto-assign a different ID, causing UTXOs to be created with ID X but the block stored with ID Y, andsetTxMinedwould still run to overwrite X→Y.Impact
setTxMinedStatusis now skipped entirely (via existing guard). UTXOs are written with the correct block ID at creation time.block.ID == 0in the gRPC request →buildAddBlockOptsreturnsnil→AddBlockbehaves exactly as before. No behaviour change.Testing
go build ./...— cleanmake lint— 0 issuesservices/legacy/netsync/...services/blockvalidation/...services/rpc/...services/asset/...ProcessBlockcall sites in integration/smoke tests updated with0(no pre-assigned ID) — no behaviour change for those testsVerification in a live environment
With LEGACYSYNCING active, the
setMinedChanworker will log:for every block processed via the legacy path, confirming
setTxMinedStatusis bypassed.