This causes crash on elements wallet when dealing with transactions that
have explicit values and confidential assets. This creates a somewhat
serious DoS attack as the sender can cause the reciever's wallet to
crash by partially blinding the change output. To make matters worse,
the wallet initially accepts the transaction, but fails while spending
the output.
This is likely caused by a combination of two bugs:
1) The wallet's current behaviour stores the complete transaction of interest
in CWalletTx instead of just Outpoints. Only that the spend time do we
iterate over all outputs, try to unblind them and check which are
isMine. When calling wtx.GetOutputValueOut() or similar calls, we hit this assertion.
While the current behaviour is okay, I think the correct way is move
the IsMine == ISMINE_NO at the start of the loop. We should not do be
any checks on outputs that are not ours. This is used in multiple
places at different parts of the codebase for different RPCs.
2) When dealing with partially blinded trasactions, ComputeBlindingData
correctly sets value = -1, and the cache byte to 1. When getting the
data again with GetBlindingData for explicit value and confidential
asset, we load the precomputed data with value = -1 and assert the
loaded value be the explicit value in the transaction. This is only true
for explicit value and explicit asset.
The changed assertion checks that written value should be same as the
explicit value that was written only when the amounts are valid
Conflicts were mostly obvious, except for those in CreateTransactionInternal.
(This function was moved from wallet.cpp to spend.cpp, and substantially
rewritten, between 0.21 and 22.0.) For those I manually applied the changes
from the diff, which wound up taking a slightly different form.
Also had to update the new test because the `addresses` field of the RPC
output was removed.
When the user specifies a change address manually, use the change address
to obtain blinding parameters (either extract the blinding key from the
address or don't blind the change).
The previous behavior would assume that the change address was owned by
the wallet and always generate a blinding key internally. If the user
were to pass a non-wallet-owned change address, the result would be an
output that could not be unblinded by its owner.
240ea294d5 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39af21 test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f16e7 wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)
Pull request description:
The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.
The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
In the context of rescanning old block, the only time value that as a meaning is the blocktime.
That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
This PR Fixes#20181.
To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).
ACKs for top commit:
jonatack:
ACK 240ea294d5 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
meshcollider:
re-utACK 240ea294d5
Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
1204b91c2d re-enable fallbackfee by default (Andrew Poelstra)
Pull request description:
Upstream disabled the default `fallbackfee` option in 0.17, which caused us some user error reports in 0.18. Re-enable the option in 0.21.
ACKs for top commit:
stevenroose:
utACK 1204b91c2d
Tree-SHA512: 62d053405c00f5ca31ae3fb24ab8def2c9e3bd52909dfac57e65224947295a56f0b8a322ffeb0dfcdcb3103fcaf310a9554f3ed3a8490859ca82aaeae26c7fb2
How to review this PR:
* Skim the description of https://github.com/bitcoin/bitcoin/issues/20347
to see that it's roughly "when pick_new_inputs is unset but bnb_used is
set, one the next iteration of the loop an assumption may be wrong"
* See that in this case, we just add another iteration to the loop...
* ...in exactly the same way as the other place that pick_new_inputs is
turned off, I just missed this one before
* Observe that the regression test fails before the patch but passes after.
Surprisingly easy to do. Almost all of the diff resolution was mechanically
* replacing boost::variant with std::variant
* replacing Optional with std::optional
* then replacing `nullopt` with `std::nullopt`
* updating the RPC functions for the new RPCArg::Default type
* update the tests/ directory to make new (since 22) tests use arrays for
createrawtransaction outputs
* other ad-hoc changes to function parameters etc (not too many of these)
I had to "really" change the code in PrecomputePSBTData, which was introduced
in 22.0 and affected by PSET, but this function was like 8 lines long so it
was easy.
Reviewing the diff may be a bit difficult because of the mix of mechanical
changes and ad-hoc things. Probably the most straightforward thing to do
is to redo the merge, `sed -i` to fix the boost::variant and Optional stuff,
then diff the remaining conflicts against this commit.
TODO: grep for `blindpsbt` and you will see that this RPC is still referenced
in documentation and help text even though it was deleted. Need to fix this
in 0.21 in a separate PR.
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
86beee0579 Use waste metric for deciding which selection to use (Andrew Chow)
b3df0caf7c tests: Test GetSelectionWaste (Andrew Chow)
4f5ad43b1e Add waste metric calculation function (Andrew Chow)
935b3ddf72 scripted-diff: tests: Use KnapsackSolver directly (Andrew Chow)
6a023a6f90 tests: Add KnapsackGroupOutputs helper function (Andrew Chow)
d5069fc1aa tests: Use SelectCoinsBnB directly instead of AttemptSelection (Andrew Chow)
54de7b4746 Allow the long term feerate to be configured, default of 10 sat/vb (Andrew Chow)
Pull request description:
Branch and Bound introduced a metric that we call waste. This metric is used as part of bounding the search tree, but it can be generalized to all coin selection solutions, including those with change. As such, this PR introduces the waste metric at a higher level so that we can run both of our coin selection algorithms (BnB and KnapsackSolver) and choose the one which has the least waste. In the event that both find a solution with the same change, we choose the one that spends more inputs.
Also this PR sets the long term feerate to 10 sat/vb rather than using the 1008 block estimate. This allows the long term feerate to be the feerate that we switch between consolidating and optimizing for fees. This also removes a bug where the long term feerate would incorrectly be set to the fallback fee. While this doesn't matter prior to this PR, it does have an effect following this. The long term feerate can be configured by the user through a new `-consolidatefeerate` option.
ACKs for top commit:
Xekyo:
reACK 86beee0 via git range-diff fe47558...86beee0
meshcollider:
re-utACK 86beee0579
Tree-SHA512: 54b154b346538eca68ae2a3b83a033b495c1605c14f842bfc43ded2256b110983ce674c647fe753cf0305b1b178403d8d60d6d4203c7a712bec784be52e90d42
The long term feerate is really the highest feerate that the user is
comfortable with making consolidatory transactions. This is should thus
be something that can be configured by the user via a new startup option
-consolidatefeerate. The default value is 10 sat/vbyte, chosen
arbitrarily (it seems like a reasonable number).
c3c213215b Use `context.args` in `src/wallet/load.cpp`. (Kiminuo)
25de4e77fe Use `context.args` in `CWallet::Create` instead of `gArgs`. (Kiminuo)
aa5e7c9471 Fix typo in bitcoin-cli.cpp (Kiminuo)
Pull request description:
The PR attempts to move us an inch towards the [goal](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615307465) by using `WalletContext` in `wallet.{h|cpp}` code instead of relying on the global state (i.e. `gArgs`).
Edit: The PR builds on #19101.
ACKs for top commit:
ryanofsky:
Code review ACK c3c213215b. Changes since last review: just rebasing and adding wallet load commit
Tree-SHA512: 2b436f5a219e32c2d529f55a89edca086d949396cebf9e089a21aa7b1c180e3c0fb17468f415dfc834f8e1614f8b3914c7e9a0bd33b95e7e0199c0dfe5ca9490
