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

Address getHash requires data length of 20 or 32 only #989

Closed
pinheadmz opened this issue Aug 13, 2020 · 2 comments · Fixed by #1038
Closed

Address getHash requires data length of 20 or 32 only #989

pinheadmz opened this issue Aug 13, 2020 · 2 comments · Fixed by #1038

Comments

@pinheadmz
Copy link
Member

So far witness v0 and proposed v1 program schemes define data lengths of either 20 or 32 bytes. However, future witness versions may be more flexible.

In particular, Address.getHash() always expects the program data to be 20 or 32 bytes.

/**
* Get the hash of a base58 address or address-related object.
* @param {String|Address|Hash} data
* @param {String?} enc - Can be `"hex"` or `null`.
* @returns {Hash}
*/
static getHash(data, enc) {
if (!data)
throw new Error('Object is not an address.');
let hash;
if (Buffer.isBuffer(data)) {
if (data.length !== 20 && data.length !== 32)
throw new Error('Object is not an address.');
hash = data;
} else if (data instanceof Address) {
hash = data.hash;
} else {
throw new Error('Object is not an address.');
}
if (enc === 'hex')
return hash.toString('hex');
return hash;
}

Since bitcoin/bitcoin#15846 "[POLICY] Make sending to future native witness outputs standard #15846" it's not entirely unreasonable to prepare for this.

Test case:

$ node
> const {Address} = require('bcoin')
> Address.fromProgram(4, Buffer.from([8,8,8]))
<Address: type=witness version=4 str=bcrt1ypqyqsvwxy99>
$ bwallet-cli send bcrt1ypqyqsvwxy99 1.0

[error] (node) Object is not an address.
    at Function.getHash (/Users/matthewzipkin/Desktop/work/bcoin/lib/primitives/address.js:811:15)
    at Wallet.readPath (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/wallet.js:894:26)
    at Wallet.syncOutputDepth (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/wallet.js:1660:31)
    at Wallet._add (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/wallet.js:1910:34)
    at async Wallet.add (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/wallet.js:1892:14)
    at async WalletDB._addTX (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/walletdb.js:2166:11)
    at async WalletDB.addTX (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/walletdb.js:2128:14)
    at async Wallet._send (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/wallet.js:1355:5)
    at async Wallet.send (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/wallet.js:1320:14)
    at async Route.handler (/Users/matthewzipkin/Desktop/work/bcoin/lib/wallet/http.js:464:18)
[debug] (net) Connecting to 100.33.69.78:10000.

Bitcoin Core doesn't mind:

$  bitcoin-cli -regtest sendtoaddress bcrt1ypqyqsvwxy99 1.0
abce2d3b778189b6e1d0e28797bda8777bb214d2e4903391edfcb2df2101481d

log:

2020-08-13T18:25:50Z [default wallet] CommitTransaction:
CTransaction(hash=abce2d3b77, ver=2, vin.size=1, vout.size=2, nLockTime=113)
    CTxIn(COutPoint(7ce70b3266, 0), scriptSig=, nSequence=4294967294)
    CScriptWitness(30440220677cabad1f488d175a3b032f7f5d3c86e4bdb578aca8cfed88daf2d208ba176a02203efed8f79b51ee662c2cf87034ebada8e1843c65898dd890f47ee3c4ed75882a01, 03ba7283769f4540b022fe80c61926db7ee0013db8114e81da6ec4c8f9f6b2cab2)
    CTxOut(nValue=1.00000000, scriptPubKey=5403080808)
    CTxOut(nValue=48.99987600, scriptPubKey=0014f27ad76d4874ad0bc057d8b3e6)
2020-08-13T18:25:50Z [default wallet] AddToWallet abce2d3b778189b6e1d0e28797bda8777bb214d2e4903391edfcb2df2101481d  newupdate
2020-08-13T18:25:50Z [default wallet] Submitting wtx abce2d3b778189b6e1d0e28797bda8777bb214d2e4903391edfcb2df2101481d to mempool for relay
2020-08-13T18:25:50Z [default wallet] AddToWallet abce2d3b778189b6e1d0e28797bda8777bb214d2e4903391edfcb2df2101481d
@pinheadmz
Copy link
Member Author

I think this could actually prevent the wallet from accepting an incoming TX if just one of the other outputs had a non-standard witness address.

@pinheadmz pinheadmz changed the title Witness program data length is not always checked against witness program version Address getHash requires data length of 20 or 32 only Aug 13, 2020
@pinheadmz
Copy link
Member Author

Just trying to figure out if we need that check in there at all. Tried removing and all tests passed, including:

$ bwallet-cli --network=regtest  send bcrt1ypqyqsvwxy99 1.0

...
  "outputs": [
    {
      "value": 100000000,
      "address": "bcrt1ypqyqsvwxy99",
      "path": null
    },
    {
      "value": 2399995860,
      "address": "mko8gJXgRjTvgAWdLYnAmfQPVkJFYUihCk",
      "path": {
        "name": "default",
        "account": 0,
        "change": true,
        "derivation": "m/0'/1/0"
      }
    }
...

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 a pull request may close this issue.

1 participant