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

fix assert crash when specified change output spend size is unknown #14380

Merged
merged 2 commits into from Nov 30, 2018

Conversation

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Oct 3, 2018

This is triggered anytime a fundraw type call(psbt or legacy) is used with a change output address that the wallet doesn't know how to sign for.

This regression was added in 6a34ff5 since BnB coin selection actually cares about this.

The fix is to assume the smallest typical spend, a P2SH-P2WPKH, which is calculated using a "prototype" dummy signature flow. Future work could generalize this infrastructure to get estimated sizes of inputs for a variety of types.

I also removed a comment which I believe is stale and misleading.

@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Oct 3, 2018

Opening this to see if people think this is the right solution.

@fanquake fanquake added the Wallet label Oct 3, 2018
@instagibbs instagibbs force-pushed the instagibbs:unknown_change_size branch Oct 3, 2018
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 3, 2018

Very nice find!

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Oct 3, 2018

The code wants to know the change signature size so it can figure out if it would have created change that was worthless to spend (because the spending will take more fees than the output is worth).

Assuming the size is smallest plausible signature size would be conservative: it'll overvalue the change, at a risk of keeping change it could have eliminated. I think doing that would be strictly superior to not using BnB at all, since preventing BnB will always keep change it should eliminate.

So, e.g. assuming any spend of an unknown type will take at least 32(txid)+4(vout)+4(sequence)+1(len)+64*0.25(signature) = 57 bytes would work. (don't assume my math is right. :) )

@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Oct 3, 2018

Hm yes I suppose you're right. Not running BnB is admitting defeat even if we actually end up being able to discard a larger value. I'll take a look at constructing something that makes a minimal segwit input weight.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Oct 4, 2018

Coverage Change (pull 14380) Reference (master)
Lines -0.0083 % 87.0471 %
Functions +0.0154 % 84.1130 %
Branches +0.0057 % 51.5403 %
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 4, 2018

Isn't the problem that coin_selection_params.change_spend_size is set to std::numeric_limits<size_t>::max() (SIZE_MAX) instead of the intended -1 since change_spend_size is of type size_t?

[cling]$ size_t change_spend_size = -1;
[cling]$ change_spend_size
(unsigned long) 18446744073709551615
[cling]$ (size_t)-1
(unsigned long) 18446744073709551615
[cling]$ #include <limits>
[cling]$ std::numeric_limits<size_t>::max()
(unsigned long) 18446744073709551615

The definition of change_spend_size:

size_t change_spend_size = 0;

CalculateMaximumSignedInputSize(…) returns -1 here:

bitcoin/src/wallet/wallet.cpp

Lines 1510 to 1520 in b012bbe

int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet, bool use_max_sig)
{
CMutableTransaction txn;
txn.vin.push_back(CTxIn(COutPoint()));
if (!wallet->DummySignInput(txn.vin[0], txout, use_max_sig)) {
// This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)
// implies that we can sign for every input.
return -1;
}
return GetVirtualTransactionInputSize(txn.vin[0]);
}

Thechange_spend_size assignment is performed here:

coin_selection_params.change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);

Related open pull requests in need of review:

  • #14252 – build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) (includes -fsanitize=integer which tackles unsigned integer wraparounds (non-UB!))
  • #14226 – mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false
  • #14224 – Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations
  • #11551 – Fix unsigned integer wrap-around in GetBlockProofEquivalentTime
  • #11535 – Avoid unintentional unsigned integer wraparounds

Please help reviewing those :-)

@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Oct 5, 2018

@practicalswift yes that's why the assert is finally hit, but it's faulty logic regardless

@instagibbs instagibbs force-pushed the instagibbs:unknown_change_size branch Oct 5, 2018
@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Oct 5, 2018

Pushed a fix in line with @gmaxwell suggestions. It's pretty hardcoded, so I'd rather make this more programmatic, perhaps giving a "hint" to the dummy signer of what type it should be?

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Oct 5, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14711 (Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky)
  • #10973 (Refactor: separate wallet from node by ryanofsky)
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@achow101
Copy link
Member

@achow101 achow101 commented Oct 5, 2018

Concept ACK

@instagibbs instagibbs force-pushed the instagibbs:unknown_change_size branch Oct 6, 2018
@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Oct 6, 2018

Updated the PR to use a function that uses DummySigner flow with an arbitrary valid pubkey as the P2SH-P2WPKH target to estimate the size more programatically.

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@instagibbs instagibbs force-pushed the instagibbs:unknown_change_size branch 2 times, most recently Oct 7, 2018
Copy link
Member

@meshcollider meshcollider left a comment

utACK 198830e

@@ -168,6 +168,9 @@ def run_test(self):
assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
assert_equal(decoded_psbt["tx"]["locktime"], 0)

# Make sure change address wallet does not have P2SH innerscript access to results in success

This comment has been minimized.

@meshcollider

meshcollider Oct 8, 2018
Member

nit: typo in comment

This comment has been minimized.

@jnewbery

jnewbery Nov 28, 2018
Member

not fixed. Suggest something like "Make sure that the wallet can successfully create a funded psbt when it isn't able to sign for the change output" or similar.

@achow101
Copy link
Member

@achow101 achow101 commented Oct 9, 2018

utACK 198830e06bf4560cfb962e29c73c5b1357d90b37

@Empact
Copy link
Member

@Empact Empact commented Oct 9, 2018

How about a higher-level test exhibiting the original failure/assert condition? Absent that we don't have regression coverage or direct failure exhibition.
Never mind! The test/functional/rpc_psbt.py change covers this.

src/wallet/wallet.h Outdated Show resolved Hide resolved
@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Oct 9, 2018

The functional test could be better explained: We need it to try BnB coin selection with a change output that is a script that it doesn't have knowledge of.

@instagibbs instagibbs force-pushed the instagibbs:unknown_change_size branch to 0fb2e69 Nov 12, 2018
@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Nov 12, 2018

rebased, just a test conflict

@@ -85,6 +85,9 @@ static const bool DEFAULT_WALLET_RBF = false;
static const bool DEFAULT_WALLETBROADCAST = true;
static const bool DEFAULT_DISABLE_WALLET = false;

//! Pre-calculated constants for input size estimation in *virtual size*

This comment has been minimized.

@Empact

Empact Nov 12, 2018
Member

nit: how about Estimated nested input size in *virtual size*, see CalculateNestedKeyhashInputSize

@Empact
Copy link
Member

@Empact Empact commented Nov 12, 2018

utACK 0fb2e69

@sipa
Copy link
Member

@sipa sipa commented Nov 14, 2018

utACK 0fb2e69

@MarcoFalke MarcoFalke added this to the 0.17.1 milestone Nov 28, 2018
@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Nov 28, 2018

utACK 0fb2e69

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Slightly tested ACK 0fb2e69. Just confirmed python test fails before and succeeds after the fix.

Copy link
Member

@jnewbery jnewbery left a comment

utACK 0fb2e69.

Minor comment nit inline, but I suggest you don't change it in order not to invalidate previous ACKs.

@@ -168,6 +168,9 @@ def run_test(self):
assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
assert_equal(decoded_psbt["tx"]["locktime"], 0)

# Make sure change address wallet does not have P2SH innerscript access to results in success

This comment has been minimized.

@jnewbery

jnewbery Nov 28, 2018
Member

not fixed. Suggest something like "Make sure that the wallet can successfully create a funded psbt when it isn't able to sign for the change output" or similar.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Nov 29, 2018

utACK

@laanwj laanwj added this to Blockers in High-priority for review Nov 29, 2018
@MarcoFalke MarcoFalke merged commit 0fb2e69 into bitcoin:master Nov 30, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
MarcoFalke added a commit that referenced this pull request Nov 30, 2018
…e is unknown

0fb2e69 CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change (Gregory Sanders)
b06483c Remove stale comment in CalculateMaximumSignedInputSize (Gregory Sanders)

Pull request description:

  This is triggered anytime a fundraw type call(psbt or legacy) is used with a change output address that the wallet doesn't know how to sign for.

  This regression was added in 6a34ff5 since BnB coin selection actually cares about this.

  The fix is to assume the smallest typical spend, a P2SH-P2WPKH, which is calculated using a "prototype" dummy signature flow. Future work could generalize this infrastructure to get estimated sizes of inputs for a variety of types.

  I also removed a comment which I believe is stale and misleading.

Tree-SHA512: c7e2be189e524f81a7aa4454ad9370cefba715e3781f1e462c8bab77e4d27540191419029e3ebda11e3744c0703271e479dcd560d05e4d470048d9633e34da16
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Nov 30, 2018

@instagibbs Would you mind creating a backport for 0.17 of this?

@instagibbs
Copy link
Member Author

@instagibbs instagibbs commented Nov 30, 2018

MarcoFalke added a commit that referenced this pull request Nov 30, 2018
…t spend size is unknown

2a5cc40 CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change (Gregory Sanders)
53dcf2b Remove stale comment in CalculateMaximumSignedInputSize (Gregory Sanders)

Pull request description:

  backport of #14380

Tree-SHA512: 42e261bd797d1938f8e041ccd10073ecd1d72695e2e4ce322e5a3ce262647e32108b01dde73361b6d2ac36438522ab3c4cd58ca072194f25011132437430cd27
@fanquake
Copy link
Member

@fanquake fanquake commented Nov 30, 2018

Backported in #14851.

@jnewbery jnewbery removed this from Blockers in High-priority for review Dec 6, 2018
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018
deadalnix added a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 23, 2020
Summary:
Remove stale comment in CalculateMaximumSignedInputSize

CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change

This is a backport of Core [[bitcoin/bitcoin#14380 | PR14380]]

Test Plan:
  ninja check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8500
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet