Encode BIP137 SegWit message signature headers - #454
Conversation
j0ntz
left a comment
There was a problem hiding this comment.
Automated code review (workflow-backed, high effort). All findings are advisory; the core BIP137 header fix is correct and matches the task requirement. Three notes below.
0c9e6a2 to
9594db2
Compare
These values have been inert until now: bitcoinjs-lib never reads network.messagePrefix, and signMessageBase64 passed no prefix, so every coin signed with bitcoinjs-message's Bitcoin default. PR #454 wires messagePrefix into the magic hash, which makes 14 of them wrong in a way that silently produces unverifiable signatures. bitcoinjs-message copies the prefix into the hash verbatim and only varint-encodes the message length, so the prefix must carry its own leading CompactSize byte. This mirrors `ss << strMessageMagic` in the coins' C++, which serializes the std::string as CompactSize(len) + bytes. Two failure classes: Wrong length byte (string was right): litecoin \x18 -> \x19 dogecoin \x18 -> \x19 feathercoin \x18 -> \x1c zcoin \x18 -> \x16 bitcoingold \x18 -> \x1d bitcoingoldtestnet \x18 -> \x1d Wrong string (mostly Bitcoin's default left in place): dash 'unused' -> '\x19DarkCoin Signed Message:\n' pivx 'PIVX Signed Message:\n' -> '\x18DarkNet Signed Message:\n' ecash 'Bitcoin Signed Message::\n' -> '\x16eCash Signed Message:\n' qtum 'Bitcoin Signed Message:\n' -> '\x15Qtum Signed Message:\n' ravencoin 'Bitcoin Signed Message:\n' -> '\x16Raven Signed Message:\n' smartcash 'Bitcoin Signed Message:\n' -> '\x1aSmartCash Signed Message:\n' ufo 'Bitcoin Signed Message:\n' -> '\x14UFO Signed Message:\n' digibyte '\x18Digibyte...' -> '\x19DigiByte Signed Message:\n' Dash's DarkCoin and PIVX's DarkNet are pre-rebrand names their Core still uses, and Vertcoin genuinely signs with Bitcoin's magic; all three are commented so they don't get "fixed" back. Sources (MESSAGE_MAGIC / strMessageMagic in each coin's Core): dash https://github.com/dashpay/dash/blob/master/src/util/message.cpp pivx https://github.com/PIVX-Project/PIVX/blob/master/src/util/validation.cpp#L27 zcoin https://github.com/firoorg/firo/blob/master/src/validation.cpp litecoin https://github.com/litecoin-project/litecoin/blob/master/src/util/message.cpp dogecoin https://github.com/dogecoin/dogecoin/blob/master/src/validation.cpp digibyte https://github.com/digibyte-core/digibyte/blob/master/src/util/message.cpp feathercoin https://github.com/FeatherCoin/Feathercoin/blob/master/src/util/message.cpp bitcoingold https://github.com/BTCGPU/BTCGPU/blob/master/src/util/message.cpp qtum https://github.com/qtumproject/qtum/blob/master/src/util/signstr.h ravencoin https://github.com/RavenProject/Ravencoin/blob/master/src/validation.cpp smartcash https://github.com/SmartCash/Core-Smart/blob/master/src/validation.cpp ufo https://github.com/UFOCoins/ufo/blob/master/src/validation.cpp ecash https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/src/common/signmessage.cpp Verified correct and left alone: bitcoin (+testnets), bitcoincash (+testnet), bitcoinsv, badcoin, groestlcoin and vertcoin. groestlcoin https://github.com/Groestlcoin/groestlcoin/blob/master/src/common/signmessage.cpp vertcoin https://github.com/vertcoin-project/vertcoin-core/blob/master/src/util/message.cpp bitcoinsv https://github.com/bitcoin-sv/bitcoin-sv/blob/master/src/validation.cpp bitcoincash https://github.com/bitcoin-cash-node/bitcoin-cash-node/blob/master/src/validation.cpp eboost is left as-is: the project is dead and its source is gone, so its magic could not be verified. Adds a test asserting every messagePrefix's leading byte matches its own length, since a mismatch is invisible until someone tries to verify a signature. Confirmed it fails when litecoin is reverted to \x18.
9594db2 to
2dcb071
Compare
|
Updated in 4cb6e2b to resolve the cross-PR conflict raised on EdgeApp/edge-react-gui#6065. BIP-137 is now opt-in rather than unconditional. Also here:
|
4cb6e2b to
d8f9bbc
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d8f9bbc. Configure here.
These values have been inert until now: bitcoinjs-lib never reads network.messagePrefix, and signMessageBase64 passed no prefix, so every coin signed with bitcoinjs-message's Bitcoin default. PR #454 wires messagePrefix into the magic hash, which makes 14 of them wrong in a way that silently produces unverifiable signatures. bitcoinjs-message copies the prefix into the hash verbatim and only varint-encodes the message length, so the prefix must carry its own leading CompactSize byte. This mirrors `ss << strMessageMagic` in the coins' C++, which serializes the std::string as CompactSize(len) + bytes. Two failure classes: Wrong length byte (string was right): litecoin \x18 -> \x19 dogecoin \x18 -> \x19 feathercoin \x18 -> \x1c zcoin \x18 -> \x16 bitcoingold \x18 -> \x1d bitcoingoldtestnet \x18 -> \x1d Wrong string (mostly Bitcoin's default left in place): dash 'unused' -> '\x19DarkCoin Signed Message:\n' pivx 'PIVX Signed Message:\n' -> '\x18DarkNet Signed Message:\n' ecash 'Bitcoin Signed Message::\n' -> '\x16eCash Signed Message:\n' qtum 'Bitcoin Signed Message:\n' -> '\x15Qtum Signed Message:\n' ravencoin 'Bitcoin Signed Message:\n' -> '\x16Raven Signed Message:\n' smartcash 'Bitcoin Signed Message:\n' -> '\x1aSmartCash Signed Message:\n' ufo 'Bitcoin Signed Message:\n' -> '\x14UFO Signed Message:\n' digibyte '\x18Digibyte...' -> '\x19DigiByte Signed Message:\n' Dash's DarkCoin and PIVX's DarkNet are pre-rebrand names their Core still uses, and Vertcoin genuinely signs with Bitcoin's magic; all three are commented so they don't get "fixed" back. Sources (MESSAGE_MAGIC / strMessageMagic in each coin's Core): dash https://github.com/dashpay/dash/blob/master/src/util/message.cpp pivx https://github.com/PIVX-Project/PIVX/blob/master/src/util/validation.cpp#L27 zcoin https://github.com/firoorg/firo/blob/master/src/validation.cpp litecoin https://github.com/litecoin-project/litecoin/blob/master/src/util/message.cpp dogecoin https://github.com/dogecoin/dogecoin/blob/master/src/validation.cpp digibyte https://github.com/digibyte-core/digibyte/blob/master/src/util/message.cpp feathercoin https://github.com/FeatherCoin/Feathercoin/blob/master/src/util/message.cpp bitcoingold https://github.com/BTCGPU/BTCGPU/blob/master/src/util/message.cpp qtum https://github.com/qtumproject/qtum/blob/master/src/util/signstr.h ravencoin https://github.com/RavenProject/Ravencoin/blob/master/src/validation.cpp smartcash https://github.com/SmartCash/Core-Smart/blob/master/src/validation.cpp ufo https://github.com/UFOCoins/ufo/blob/master/src/validation.cpp ecash https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/src/common/signmessage.cpp Verified correct and left alone: bitcoin (+testnets), bitcoincash (+testnet), bitcoinsv, badcoin, groestlcoin and vertcoin. groestlcoin https://github.com/Groestlcoin/groestlcoin/blob/master/src/common/signmessage.cpp vertcoin https://github.com/vertcoin-project/vertcoin-core/blob/master/src/util/message.cpp bitcoinsv https://github.com/bitcoin-sv/bitcoin-sv/blob/master/src/validation.cpp bitcoincash https://github.com/bitcoin-cash-node/bitcoin-cash-node/blob/master/src/validation.cpp eboost is left as-is: the project is dead and its source is gone, so its magic could not be verified. Adds a test asserting every messagePrefix's leading byte matches its own length, since a mismatch is invisible until someone tries to verify a signature. Confirmed it fails when litecoin is reverted to \x18.
signMessageBase64 always emitted a legacy header byte (27-34) regardless of the signing address type, so BIP137 verifiers (e.g. Bringin) rejected signatures from SegWit addresses. Thread the address format through and select the matching segwitType: bip49 -> p2sh(p2wpkh) (header 35-38), bip84 -> p2wpkh (header 39-42), bip44/bip32 unchanged (31-34). Also magic-hash with each coin's own messagePrefix rather than always Bitcoin's, so the signature verifies against that coin's addresses.
2bc9171 to
10d018f
Compare


CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Description
Asana task
Edge's BTC message signing (used for CEX/ramp withdrawal ownership proofs, e.g. Bringin / NiceHash Travel Rule) produced a signature whose header byte always encoded a legacy P2PKH type (27-34), regardless of the signing address. BIP137 verifiers reject a legacy-header signature that comes from a SegWit address: nested SegWit (P2SH-P2WPKH) requires header 35-38 and native SegWit (P2WPKH / bech32) requires 39-42. Because Edge BTC wallets default to SegWit, the signatures Edge emitted were rejected by Bringin; a user had to run a script to rewrite the header to BIP137 before it was accepted, or fall back to selfie/ID verification.
signMessageBase64inkeymanager.tscalledbitcoinMessage.sign(message, privKey, compressed)with nosegwitType, so the header byte only reflected the compressed flag. This threads the address's derivationformatthrough and selects the matchingsegwitType:bip84(native SegWit / P2WPKH) -> header 39-42bip49(nested SegWit / P2SH-P2WPKH) -> header 35-38bip44/bip32(legacy P2PKH) -> header 31-34 (unchanged)The header selection routes through the canonical
BIP43PurposeTypeEnumrather than raw format string literals, so a newly-added SegWit format fails loudly in the purpose mapping instead of silently falling through to a legacy header.The ECDSA signature is unchanged for legacy P2PKH signing on Bitcoin; only the header byte differs there. The fix is general across all UTXO coins that support SegWit, since
path.formatreflects the actual address derivation.BIP137 is opt-in
asUtxoSignMessageOtherParamscarriessignatureFormat: 'electrum' | 'bip137', defaulting toelectrum.segwitTypeis applied only whenbip137is requested, so existing callers (bityProvider,EdgeProviderServer) keep the header byte they produce today. Emitting BIP137 unconditionally would have been a silent behavior change for them, and it would have broken EdgeApp/edge-react-gui#6065 depending on merge order: that PR's "Standard (Electrum)" option would have started emitting BIP137 headers. Making it opt-in removes the ordering hazard, and the GUI now sends the option instead of rewriting the header itself.Scoped purpose lookup
bip43PurposeNumberToTypeEnumthrows on a purpose it does not map, so resolving it unconditionally would let a futureCurrencyFormat(bip86 Taproot being the obvious candidate) break message signing on the default Electrum path, which never needs it. The lookup lives in agetBip137SegwitTypehelper that returns before touching it unless BIP137 is requested, so an unmapped format can only affect BIP137 signing.AddressNotOwnedErrorsignMessagethrows a namedAddressNotOwnedErrorwhen the wallet cannot sign for the requested address, whether it fails to parse or simply is not ours. Callers branch onerror.namerather than matching parser prose, following the existingasMaybeInsufficientFundsErrorPluspattern.Coin message prefixes
The header byte alone is not enough for non-Bitcoin coins.
signMessageBase64previously magic-hashed withbitcoinjs-message's default"Bitcoin Signed Message:\n"for every coin, so an altcoin signature carried the right header but the wrong prefix and would not verify against that coin's own addresses. It now hashes with the coin's ownprefixes.messagePrefix[0].That made the prefix data load-bearing for the first time, and an audit by @peachbits (commit absorbed here, authored by them and ordered first) found 14 coins wrong.
bitcoinjs-messagecopies the prefix into the hash verbatim and only varint-encodes the message length, so the prefix must carry its own leading CompactSize byte, mirroringss << strMessageMagicin each coin's C++. Two failure classes: a wrong length byte (litecoin, dogecoin, feathercoin, zcoin, bitcoingold and its testnet) and a wrong string, usually Bitcoin's default left in place (dash, pivx, ecash, qtum, ravencoin, smartcash, ufo, digibyte). Dash'sDarkCoin, PIVX'sDarkNet, and Vertcoin's genuine use of Bitcoin's magic are pre-rebrand or intentional and are commented so they do not get "fixed" back.Testing
H…header forbip44,bip49andbip84, and only an explicitbip137request shiftsbip49toI…andbip84toJ…. 12 cases, so swapping the two formats fails the suite.tsc,eslint, and the fullmochasuite pass (1251 tests);npm run verifypasses.messagePrefix.spec.tsasserts every registered coin's prefix leading byte matches its string length, covering 24 coins. This is the guard that makes a silently-unverifiable prefix a build failure.bitcoinjs-message.verifyagainst the derived legacy, P2SH-P2WPKH, and bech32 LTC addresses, and are confirmed NOT to verify under Bitcoin's prefix, so a regression back to the default would fail the suite.bitcoinjs-message.verifyagainst its real P2PKH / P2SH-P2WPKH / bech32 address.EdgeCurrencyWallet.signMessagepath with this build linked, on the wallet's own receive addresses. The nested-SegWit3...address produced header 35 and the native-SegWitbc1q...address produced header 40, both verifying against their respective addresses (screenshot attached). Before the fix both emitted header 31, which fails BIP137 verification for a SegWit address.updot): signing the same message with bip84 addressbc1q7f5wkp5k0847utsec7p8v59cqj7et6wrzz7dv4produced header 32 under the default and header 40 undersignatureFormat: 'bip137', identical r/s, both verifying against that address viabitcoinMessage.verify(..., checkSegwitAlways: true). That confirms the default no longer emits a SegWit header.Asana: https://app.asana.com/1/9976422036640/project/1215088146871429/task/1216403654258303
Note
Medium Risk
Changes message-signing output for altcoins (prefix fixes) and opt-in BIP137 headers; default Electrum behavior is preserved but wrong-prefix fixes alter signatures that may have been used in production for non-BTC chains.
Overview
UTXO message signing is extended so ramp/CEX ownership proofs (e.g. BIP137 verifiers) can succeed without clients rewriting signatures.
signMessageaccepts optionalsignatureFormat: 'electrum' | 'bip137'(defaultelectrum). Withbip137,signMessageBase64setsbitcoinjs-messagesegwitTypefrom the address derivation path (bip49→ nested SegWit,bip84→ native SegWit). Signing now uses each coin’smessagePrefixinstead of Bitcoin’s default, and 14 altcoins had wrong prefix strings/length bytes corrected.signMessagethrowsAddressNotOwnedErrorwhen the address is invalid for the chain or not owned by the wallet (replacing generic errors).Tests add Bitcoin/Litecoin vectors for both encodings and
messagePrefix.specguards prefix CompactSize bytes across all registered coins.Reviewed by Cursor Bugbot for commit 10d018f. Bugbot is set up for automated code reviews on this repo. Configure here.