Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bech32m support #1038

Merged
merged 8 commits into from Nov 12, 2021
Merged

Bech32m support #1038

merged 8 commits into from Nov 12, 2021

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Oct 11, 2021

Merging work from #1034 and #1035
Closes #989

Reviewers can ignore the first two commits, the backend bech32m stuff is easier to review here:
bcoin-org/libtorsion#1
bcoin-org/bcrypto#61

The most important new function here is the ability to send BTC to new Taproot addresses, which use a new format called bech32m. I tested this branch by networking bcoin and Bitcoin Core together in regtest and sending BTC from bcoin to a Taproot wallet in Bitcoin Core:

  1. This gist explains how to start Bitcoin Core in regtest and create a taproot wallet.

  2. Start bcoin in regtest and connect:

export BCOIN_NETWORK=regtest
bcoin --daemon
bcoin-cli rpc addnode 127.0.0.1:18444 add
  1. Generate regtest coins to bcoin wallet:
bcoin-cli rpc generatetoaddress 10 `bwallet-cli rpc getnewaddress`
  1. Send to Bitcoin Core wallet:
bwallet-cli send bcrt1pnmrmugapastum8ztvgwcn8hvq2avmcwh2j4ssru7rtyygkpqq98q4wyd6s 1.12345678
  1. Check Bitcoin Core wallet balance:
bitcoin-cli -regtest -rpcwallet=taproot-test-1 getbalance

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #1038 (ded6e5e) into master (1a8cd52) will increase coverage by 0.10%.
The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
+ Coverage   65.40%   65.51%   +0.10%     
==========================================
  Files         156      156              
  Lines       26075    26100      +25     
==========================================
+ Hits        17055    17099      +44     
+ Misses       9020     9001      -19     
Impacted Files Coverage Δ
lib/node/node.js 75.71% <ø> (ø)
lib/protocol/network.js 83.07% <66.66%> (-0.39%) ⬇️
lib/primitives/address.js 89.92% <97.22%> (+2.33%) ⬆️
lib/protocol/networks.js 100.00% <100.00%> (ø)
lib/indexer/addrindexer.js 97.67% <0.00%> (-2.33%) ⬇️
lib/mining/miner.js 69.39% <0.00%> (+0.86%) ⬆️
lib/mempool/fees.js 58.53% <0.00%> (+3.93%) ⬆️
lib/script/program.js 79.16% <0.00%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a8cd52...ded6e5e. Read the comment docs.

lib/primitives/address.js Outdated Show resolved Hide resolved
lib/primitives/address.js Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@pinheadmz
Copy link
Member Author

In addition to the Address module, the bech32m update meant we also had to fix a problem with the address indexer.

The indexer was added in #758 and claimed to close issue #738 which is described as follows:

Only the address hash is used in the index, so there is the output type is not included between the various output types. This has the result that bech32 (segwit) and base58 (non-segwit) addresses that have the same hash share the same results.

The indexer module is exceptional work and solved a lot of problems including this one, however it effectively recreated the exact same problem for future witness versions. In this article the author notes that several taproot-formatted addresses exist on chain already (they are anyone-can-spend until taproot activates). One of those addresses was actually sent by me:

bc1pqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqs3wf0qm

This is technically a version 1 witness address (although it is bech32 and not bech32m, that doesn't matter once the utxo is on-chain). The data (hash) for this address is a joke of sorts:

010​10101​01010101​010​101010101010​10​10​101010101​0101010101​0​101010​101

If you are currently running bcoin with --index-address this address would be indexed but improperly stored with the virtual prefix for p2wsh. That means that if someone sends a transaction to a version 0 address with the same data (hash), those two addresses will collide in the indexer. The version 0 collision address is:

bc1qqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqsw9e2a9

So these two different addresses will be indexed as the same in the current code, meaning "get TX by address" results could extraneously include transactions including the colliding address.

This is an edge case for sure, since neither of these addresses are spendable. An accidental collision with usable addresses would be really insane (a version 0 script hash and a version 1 taproot program). However we have no idea how realistic a collision like this will be with future witness versions and program updates.

The fix for this bug is in cca5243 and the answer is to include witness version in the address indexer. For some reason, we were not already doing this. What we were doing was using phony "address prefixes" that made witness addresses work backwards-compatibly with legacy addresses (which have actual prefixes like 1 and 3 for mainnet pubkey and script hash addresses, respectively).

Since there are a handful of witness version 1 programs already on chain, anyone running bcoin with address indexer technically has corrupted data. The longer users wait to upgrade, the more corrupt data will be stored in the address indexer. This will start to get especially bad after taproot activates.

This makes it pretty important to merge this bug fix before taproot activation and minimize the impact. Since users may not actually upgrade their node before activation, we will need a data migration script to fix version 1 programs inadvertently stored as version 0 in the indexer.

lib/primitives/address.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pinheadmz and others added 2 commits November 8, 2021 13:47
Co-authored-by: Nodari Chkuaselidze <nodar.chkuaselidze@gmail.com>
@martindale
Copy link

Well played, sir. Haven't done code review, but would love to see merges from Handshake + Taproot support make it into a 3.0. Feel free to share a Bounty URL for faster activation...

@pinheadmz
Copy link
Member Author

ACK d6a13b0

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK d6a13b01854f100a39c536bebf4d61aeacfd638e
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmGOkF4ACgkQ5+KYS2KJ
yTqNSxAAj/5pAs6M/UUgh5D2GEreccgHTGM8ds7wFuRn2iQJZTTLkrgOeJs9T3ww
dDbFGBTt/HhlYkX6ArocClm7k7VTKTL+tQ+609VP7y+L0xaScs7UA69XW/F+wPE8
QF8j2sxPl4wWr3wFQtx6sAZDkwrRtggbgRTHSBMUwzQQmYe4HugHdljRB/NgBXi0
DAOhmEpmsXHg7FME3D3V/d/UEZ8d71dEQ7bxdvqypgeERw104rqPKjqdIgp6kqrn
hFIMF3V1IVW/BU41t3m1z1cS/II/mL/Qpfbcv0VRXqmTVjTTScnN1todZmUKgdt2
4Rocz3kE9LC4K+HxDQ1nUajwl0KCUvF2Lg3Pn8XjzcrzCBlNfD3k8P4EKMrQu9oX
VjesakHj2NlCsq2w45xRp1WY+xh6OZedBMJRV+3vaG4urvYf9gACHCy77u8uDZra
lBOaV57GZ3PFIzEYlahOFZkDR38JCueU3/zjMWgLKh9SEl9Ca1dXjkBuIKJx95DA
JagPWhyAXN6wnmkriXV5KkZTYFFrkXTmiPq6pOUaSpKgCK+F+zIWbm+DZ/WxjmFU
lZ9x4qastt3L/BM6FP24h6ODAvbuA4G2t5JNh9/7+PLvvXeegiQcLAqsNWaKF7oo
4EttJX6x9YIuMUfmtdAQU9jGV254a+qs406IiUvhcBzYlWpFmxU=
=NGbr
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address getHash requires data length of 20 or 32 only
6 participants