[RDY] Update Bcash to support native tokens and P2SH32 address type - #1309
[RDY] Update Bcash to support native tokens and P2SH32 address type#1309mainnet-pat wants to merge 4 commits into
Conversation
Add P2SH32 support Update txdetails template for BCH
|
Thank you for the contribution, we will take a look and review it! |
Thanks. While working further on BCH native token support in blockbook I realized that I'd need to reapproach the way I handle them (will commit soon). CashTokens live in output's PubkeyScript prepending the classical Might sound a bit too technical, if you need any extra info, DM me on TG or discord @mainnet_pat |
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades Bcash support to include native tokens (CashTokens) and P2SH32 address type support, implementing comprehensive token data parsing and rendering functionality for the Bitcoin Cash network.
- Updates to
bchutilpackage for P2SH32 address support - New token parsing infrastructure with comprehensive validation
- Frontend updates to display CashTokens with color-coded categories
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| bchain/types_bcash_type.go | Defines new BCH-specific data structures for CashTokens and NFTs |
| bchain/coins/bch/bcashparser.go | Core token parsing implementation with full validation |
| db/rocksdb.go | Updates address key packing to handle token prefixes |
| api/worker.go | Integrates token parsing into transaction processing |
| server/public.go | Adds template function for token rendering with color-coding |
| static/templates/txdetail.html | Template updates to display token information |
| static/css/main.css | New CSS classes for token display styling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /** | ||
| * Given a 32-byte hex-encoded token category, return a deterministic hue and | ||
| * saturation value to use in HSL colors representing the token category. | ||
| * Usage: `hsl(${ tokenCategory2HueSaturation(vout.tokenData.category) }, 50%)` |
There was a problem hiding this comment.
The documentation comment uses JavaScript-style template literal syntax in the usage example, but this is a Go function. The usage example should show Go template syntax instead.
| * Usage: `hsl(${ tokenCategory2HueSaturation(vout.tokenData.category) }, 50%)` | |
| * Usage: `hsl({{ tokenCategory2HueSaturation .Vout.TokenData.Category }}, 50%)` |
| // see original implementation: https://github.com/sickpig/bch-rpc-explorer/blob/85aed82dcae5b22050f26184bde229fe60a958b9/app/utils.js#L812 | ||
| func tokenCategory2HueSaturation(category string) string { | ||
| // Decode hex string to bytes | ||
| bin, _ := hex.DecodeString(category) |
There was a problem hiding this comment.
Error from hex.DecodeString is ignored. While this might be acceptable if the category is guaranteed to be valid hex, it's better practice to handle the error or add a comment explaining why it's safe to ignore.
| bin, _ := hex.DecodeString(category) | |
| bin, err := hex.DecodeString(category) | |
| if err != nil { | |
| // Return a default value if decoding fails | |
| return "0,50%" | |
| } |
| // TODO: do not panic | ||
| panic(errors.Errorf("Cannot get script from address descriptor %s", addrDesc)) |
There was a problem hiding this comment.
Using panic for error handling is generally not recommended in production code. Consider returning an error instead or implementing proper error handling.
| if br.Size() < 34 { | ||
| return nil, 0, fmt.Errorf("Invalid token prefix: insufficient length. The minimum possible length is 34. Missing bytes: %d", 34-br.Size()) |
There was a problem hiding this comment.
The calculation 34-br.Size() is incorrect. Since br.ReadByte() was already called, br.Size() returns the original size, not the remaining size. This should be 34-len(script) or use br.Len() for remaining bytes.
| if br.Size() < 34 { | |
| return nil, 0, fmt.Errorf("Invalid token prefix: insufficient length. The minimum possible length is 34. Missing bytes: %d", 34-br.Size()) | |
| if br.Len() < 33 { | |
| return nil, 0, fmt.Errorf("Invalid token prefix: insufficient length. The minimum possible length is 34. Missing bytes: %d", 33-br.Len()) |
| hueSat := tokenCategory2HueSaturation(t.Category) | ||
| tokenColor := "hsl(" + hueSat + ",40%)" | ||
|
|
||
| // outter div |
There was a problem hiding this comment.
Corrected spelling of 'outter' to 'outer'.
| rv.WriteString(`</div>`) | ||
|
|
||
| } | ||
| // outter div end |
There was a problem hiding this comment.
Corrected spelling of 'outter' to 'outer'.
|
I am glad to announce that I have finished the work on CashTokens support in blockbook. Token information is nicely parsed from outputs and stored in Final sync is being completed and is available at https://blockbook.pat.mn Please let me know if you have any questions so we can finalize this PR towards the inclusion in the mainline. Images |
Look forward to get your feedback on the latest work |
pragmaxim
left a comment
There was a problem hiding this comment.
That's a lot of great work here! I made a few comments, cheers
| if d.is.IsBCH() && len(buf[al:]) > 0 { | ||
| tokenLen, tl := unpackVarint(buf[al:]) | ||
| al += tl | ||
| if tokenLen > 0 && len(buf[al:]) >= tokenLen { | ||
| bcashToken, ll, err := bch.UnpackTokenData(buf[al:]) | ||
| if err == nil && ll == tokenLen { | ||
| al += ll | ||
| ti.BcashToken = bcashToken | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Just fyi, this is not binary compatible and db must be resynced. DB format changes without a version bump will misparse existing DBs.
- Why it matters: BCH nodes with an existing DB will read old
txAddresses/addressBalancerows with the new token-length fields, leading to silent data corruption or misaligned reads. - Suggested fix (force reindex or migrate):
If you want a migration path, add a 7->8 migration in
// db/rocksdb.go const dbVersion = 8
checkColumns(similar to 6->7) and gate token fields behind that version.
| } | ||
|
|
||
| func unpackAddrBalance(buf []byte, txidUnpackedLen int, detail AddressBalanceDetail) (*AddrBalance, error) { | ||
| func (d *RocksDB) unpackAddrBalance(buf []byte, txidUnpackedLen int, detail AddressBalanceDetail) (*AddrBalance, error) { |
There was a problem hiding this comment.
I think that unpackAddrBalance leaks the previous token into subsequent non-token UTXOs.
- Why it matters: a single token UTXO followed by a normal UTXO will cause the later UTXO to incorrectly carry the token metadata.
- Suggested fix (reset token per loop):
for len(buf[l:]) >= txidUnpackedLen+3 { var bcashToken *bchain.BcashToken // ... parse fields ... if d.is.IsBCH() && len(buf[l:]) > 0 { // parse tokenLen/token data into bcashToken } u := Utxo{BcashToken: bcashToken, /* ... */ } // ... }
| if d.is.IsBCH() && len(buf[al:]) > 0 { | ||
| tokenLen, tl := unpackVarint(buf[al:]) | ||
| al += uint(tl) | ||
| if tokenLen > 0 && len(buf[l:]) >= tokenLen { | ||
| bcashToken, ll, err := bch.UnpackTokenData(buf[al:]) | ||
| if err == nil && ll == tokenLen { | ||
| al += uint(ll) | ||
| ti.BcashToken = bcashToken | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
BCH tx input decoding uses the wrong bounds check for token data in the non-extended path.
- Why it matters: the
len(buf[l:])check can be true even when there is not enough data remaining, which can misalign the decoder on corrupted/partial rows (or after a failed migration). - Suggested fix:
if tokenLen > 0 && len(buf[al:]) >= tokenLen { // ... }
| if d.is.IsBCH() { | ||
| script, _ := hex.DecodeString(output.ScriptPubKey.Hex) | ||
| var l int | ||
| bcashToken, l, err = bch.UnpackTokenData(script) | ||
| if err == nil { | ||
| addrDesc = script[l:] | ||
| } | ||
| } else { | ||
| addrDesc, err = d.chainParser.GetAddrDescFromVout(output) | ||
| } |
There was a problem hiding this comment.
BCH output parsing skips P2PK normalization and ignores script decode errors.
- Why it matters: P2PK outputs are no longer normalized to P2PKH when tokens are present, which changes address indexing behavior compared to legacy BCH handling.
- Suggested fix (parse token + use parser normalization):
if d.is.IsBCH() { script, err := hex.DecodeString(output.ScriptPubKey.Hex) if err != nil { return err } bcashToken, _, err = bch.UnpackTokenData(script) if err != nil { return err } addrDesc, err = d.chainParser.GetAddrDescFromVout(output) }
| if len(metaQueue) > 0 { | ||
| serializedMetaQueue := make([][]byte, len(metaQueue)) | ||
| for i, mq := range metaQueue { | ||
| serializedMetaQueue[i] = append(PackBcashTokenMetaQueueKey(mq, make([]byte, 34)), PackBcashTokenMetaQueue(mq, make([]byte, 9))...) | ||
| } | ||
| common.BcmrMetaQueueSignal <- serializedMetaQueue | ||
| } |
There was a problem hiding this comment.
processBcashTokens can block block processing on slow BCMR downloads.
- Why it matters: the channel has a depth of 1 and the consumer performs synchronous HTTP fetches; this can stall indexing under load or provider latency.
- Suggested fix (non-blocking send or decouple):
select { case common.BcmrMetaQueueSignal <- serializedMetaQueue: default: glog.Warning("BCMR queue signal dropped; downloader is busy") }
| categoryName := categoryHex | ||
| if token.Name != "" { | ||
| categoryName = token.Name | ||
| } | ||
|
|
||
| tokenCategoryImg := "" | ||
| if token.Icon != "" { | ||
| tokenCategoryImg = fmt.Sprintf(`<img onerror="this.style.display='none'" src="%s" alt="Token Icon" width="32px" height="32px"> `, token.Icon) | ||
| } |
There was a problem hiding this comment.
BCMR metadata is rendered into HTML without escaping, enabling XSS via token metadata.
- Why it matters:
token.Name,token.Icon, and NFT metadata are external inputs that can contain HTML/JS. - Suggested fix (escape user-controlled fields or avoid
template.HTML):categoryName := template.HTMLEscapeString(token.Name) tokenCategoryImg := template.HTMLEscapeString(token.Icon) // ... and avoid returning template.HTML for raw concatenated markup
| } | ||
| } | ||
|
|
||
| func Test_packAddrBalance_unpackAddrBalance_Bcash(t *testing.T) { |
There was a problem hiding this comment.
tests do not cover token UTXO followed by non-token UTXO in address balance decoding.
- Why it matters: this gap would allow the token carry-over bug.
- Suggested fix: add a test vector where a token UTXO is followed by a non-token UTXO and ensure the latter has
BcashToken == nil.
|
Amazing review and findings, thank you. Glad you liked my work. |
|
@mainnet-pat I made review and you thanked me but did not react on the comments, now there are conflicts since the PR is stale, we were happy to merge this but you did not address the comments |




This pull request upgrades the Bcash to support new BCH network upgrades to include P2SH32 address type support and BCH native tokens (CashTokens) support.
bchutilpackage was updated but referenced as a replacement package. A separate PR for it is out of scope for this review, but can be filed immediately .Blockbook's
bcashparser.gowas updated to support new P2SH32 addresses frombchutiland to support token info parsing from output locking bytecode. Extensive tests were added tobcashparser_tests.goNew token type structs were put into
types_bcash_type.gorocksdb.gofunctionpackAddressKeywas updated to strip token info fromAddressDescriptorfor BCH case and store all CashToken utxos under the same key.worker.gofunctionsgetTransactionFromBchainTxandGetTransactionFromMempoolTxwere updated to return BCH specific token data.txdetail.htmltemplate andpublic.gowere updated to render CashTokens specific data with different token categories being color-coded accordingly as there are more than one different tokens allowed to be interacted with within a single transaction.go.sumupdates are unintentional and made by my IDE to compile the project, let me know if we can sort this out with minimal interventions.This PR looks for repository's maintainer feedback regarding data structures and other general remarks which will update its quality.
This PR is marked as WIP, as currently the token metadata (ticker, name, decimals, etc.) lookup and rendering are not implemented, and will be worked on after the submission.
Deployment preview:
Current:
https://bch1.trezor.io/tx/d7ad5f6336cd2c80c86e241ab2deb7b981b7dd9234b68a5b42e304829e482fb2
https://bch1.trezor.io/api/v2/tx/d7ad5f6336cd2c80c86e241ab2deb7b981b7dd9234b68a5b42e304829e482fb2
This PR:
https://blockbook.pat.mn/tx/d7ad5f6336cd2c80c86e241ab2deb7b981b7dd9234b68a5b42e304829e482fb2
https://blockbook.pat.mn/api/v2/tx/d7ad5f6336cd2c80c86e241ab2deb7b981b7dd9234b68a5b42e304829e482fb2
MfG,
pat.