toggle to allow raw display of miner tag as per WOC#441
Merged
freemans13 merged 13 commits intoJun 5, 2026
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete No issues found. This PR implements a clean, well-tested feature with strong documentation. Review Summary:
|
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-06-04 17:34 UTC |
|
|
|
||
| func runConfigChecks(s *settings.Settings) []ConfigResult { | ||
| var results []ConfigResult | ||
| results := make([]ConfigResult, 0, 16) //nolint:prealloc |
Contributor
There was a problem hiding this comment.
The //nolint:prealloc comment is confusing. This code IS preallocating with make([]ConfigResult, 0, 16). The prealloc linter suggests preallocating slices - which you are doing.
The linter might be suggesting make([]ConfigResult, 16) instead of make([]ConfigResult, 0, 16) since items will be appended. The nolint suppresses that, but the naming is misleading.
Suggestion: Either remove //nolint:prealloc or add a clarifying comment like // Using append pattern for clarity.
Update: This issue is still present in the current code.
|
|
||
| func runHealthChecks(ctx context.Context, logger ulogger.Logger, s *settings.Settings) []HealthResult { | ||
| var results []HealthResult | ||
| results := make([]HealthResult, 0, 8) //nolint:prealloc |
Contributor
There was a problem hiding this comment.
Same issue here - the //nolint:prealloc comment is misleading since the code IS preallocating the slice.
Suggestion: Either remove the nolint directive or add a clarifying comment.
Update: This issue is still present in the current code.
| // serializeBlock serializes a complete block (header + transactions) to hex string | ||
| func (bc *BlockCreator) serializeBlock(header *BlockHeader, txs []*bt.Tx) string { | ||
| var buf []byte | ||
| buf := make([]byte, 0, 256) //nolint:prealloc |
Contributor
There was a problem hiding this comment.
Same issue - //nolint:prealloc is misleading when the code IS preallocating.
Suggestion: Either remove the nolint directive or add a clarifying comment.
Update: This issue is still present in the current code.
oskarszoon
approved these changes
May 14, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Approve. Clean toggle gated by blockchain_raw_miner_tag defaulting false, real coinbase fixtures in the new tests, no behaviour change for existing deployments.
Three things worth checking before merge:
- PR description says
services/asset/centrifuge_impl/centrifuge.gois updated but that file isn't in the diff. Centrifuge websocket notifications still go throughExtractCoinbaseMiner(norawparameter), so they'll stay on the sanitized variant regardless of the setting. Intentional, or a missed call site? testCI job is failing on the latest run while everything else is green (same pattern as a couple of unrelated PRs this week). Worth a re-run to confirm flake vs real.cmd/diagnose/{config,health}_checks.go+test/utils/svnode/blockcreator.goprealloc fixes look unrelated — bundled in via rebase? Fine to leave but normally separate-PR territory.
- Revert unrelated //nolint:prealloc changes in cmd/diagnose and test/utils/svnode (scope creep; main already uses `var results`, and CI disables the prealloc linter so the preallocation was never required). - Fix TestExtractCoinbaseMinerRawPreservesAllBytes: it ignored its own table and ran identical hardcoded logic 3x. Now drives each case from the table and asserts exact bytes for raw vs sanitized modes. - Add TestExtractCoinbaseMinerRawJSONRoundTrip documenting that raw bytes become U+FFFD over JSON (matching how WhatsOnChain renders them), so the encoding behaviour is pinned. - Add TestRawMinerTagSettingWiring verifying the SQL store honours blockchain_raw_miner_tag end-to-end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
liam
approved these changes
Jun 5, 2026
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.
What this does
Adds a setting,
blockchain_raw_miner_tag(defaultfalse), that turns off the cleanup Teranode normally applies to the miner tag read from a block's coinbase transaction.This is for TAAL, who want to compare Teranode's miner tags against WhatsOnChain. WhatsOnChain shows the raw coinbase text; Teranode cleans it up, so the two don't line up.
Fixes https://github.com/bitcoin-sv/teranode/issues/1298
The problem
For mainnet block 514587:
A% Zj?d?;?�^G^GA% Zj�d�;��The difference is that Teranode strips out non-printable characters and trims the tag, while WhatsOnChain leaves the raw bytes in.
How it works
When
blockchain_raw_miner_tag=false(the default), nothing changes. Miner tags are cleaned up exactly as before:/is dropped/is droppedWhen
blockchain_raw_miner_tag=true, the coinbase text is returned as-is, with no cleanup.One thing to be aware of: JSON encoding
The miner tag is sent to clients as JSON (over the HTTP API and WebSocket notifications). Raw coinbase bytes are often not valid UTF-8, and Go's JSON encoder replaces any invalid bytes with the Unicode replacement character (
�). So clients do not get the exact original bytes back — they get a string where the invalid bytes have become�.That happens to be the same thing WhatsOnChain shows (its
�characters are the same replacement character). So this gets us close to WhatsOnChain's display, but it is not a byte-for-byte copy of the coinbase. If byte-exact data were ever needed we'd have to expose it as hex or base64 — but that wouldn't match WhatsOnChain, so it is deliberately not done here.TestExtractCoinbaseMinerRawJSONRoundTrippins this behaviour down so it can't change unnoticed.Where the change lives
The setting only affects how the SQL blockchain store extracts the miner tag. Everything that displays a miner name (the HTTP block-header endpoints, and any WebSocket notification that carries it) gets its value from the store's block header metadata, so they all pick up the setting automatically. No other code needed changing.
Changing the setting
The store reads the setting once at startup, so changing
blockchain_raw_miner_tagrequires a restart to take effect.Code changes
blockchain_raw_miner_taginBlockChainSettings(defaultfalse).ExtractCoinbaseMinerRaw(coinbaseTx, raw bool)inutil/coinbase.go. The existingExtractCoinbaseMiner()is kept and now just calls the new function withraw=false, so existing callers are unchanged.s.rawMinerTagand passes it at every miner-extraction point:block_helpers.go,GetBlockHeader.go,GetBlockHeaders.go,GetBestBlockHeader.go,GetLatestHeaderFromBlockLocator.go.Tests
TestExtractCoinbaseMinerRaw— checks raw vs sanitized output.TestExtractCoinbaseMinerRawPreservesAllBytes— checks raw mode keeps null bytes, high bytes, and control characters, while sanitized mode strips them.TestExtractCoinbaseMinerRawJSONRoundTrip— documents and pins the JSON replacement-character behaviour described above.TestRawMinerTagSettingWiring(SQL store) — checks the setting is honoured end-to-end through the store.How to turn it on