Currently, the test is passing due to a mistake in the test inputs
selection process. We are selecting the parent transaction change
output as one of the inputs of the transaction to fund, which
helps to surpass the target amount when it shouldn't due to the
fee reduction.
The failure arises when the test behaves as intended by its coder;
that is, when it does not select the change output. In this case,
the pre-selected inputs aren't enough to cover the target amount.
Fix this by excluding the parent transaction's change output from
the inputs selection and including an extra input to cover the tx
fee.
98570fe29bb08d7edc48011aa6b9731c6ab4ed2e test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d193b91ca42f18759098d3fef6eaab05 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner)
Pull request description:
Parsing legacy public keys can fail for three reasons (in this order):
- pubkey is not in hex
- pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively)
- pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check)
Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user.
Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`.
Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific.
The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before.
ACKs for top commit:
stratospher:
tested ACK 98570fe.
davidgumberg:
ACK 98570fe29b
Eunovo:
Tested ACK 98570fe29b
achow101:
ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
This deduplicates code and leads to more consistent and detailed error
messages. Affected are legacy import RPCs (`importpubkey`,
`importmulti`) and other ones where solving data can be provided
(`fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendall`).
If the serialized transaction passed to `fundrawtransaction` contains
duplicates, they will be deserialized and added to the transaction. Add
a test to ensure this behavior is not changed during the refactor.
A user can pass any number of duplicated and unrelated addresses as an
SFFO argument to `sendmany` and the RPC will not throw an error (note,
all the rest of the RPCs which take SFFO as an argument will error if
the user passes duplicates or specifies outputs not present in the
transaction). Add a test to ensure this behavior is not changed during
the refactor.
This commit introduces a helper `create_outpoints` to execute the
`send` RPC and immediately return the target address outpoints as UTXO
dictionary in the common format, making the tests more readable and
avoiding unnecessary duplication.
004903ebade38ba47c5ddc17b756d605b963528e test: Add Wallet Unlock Context Manager (Brandon Odiwuor)
Pull request description:
Fixes#28601, see https://github.com/bitcoin/bitcoin/pull/28403#discussion_r1325426430
Add Context Manager to manage the locking and unlocking of locked wallets with a passphrase during testing.
ACKs for top commit:
kevkevinpal:
lgtm ACK [004903e](004903ebad)
maflcko:
lgtm ACK 004903ebade38ba47c5ddc17b756d605b963528e
Tree-SHA512: ab234c167e71531df0d974ff9a31d444f7ce2a1d05aba5ea868cc9452f139845eeb24ca058d88f058bc02482b762adf2d99e63a6640b872cc71a57a0068abfe8
The external input weight test in wallet_fundrawtransaction.py made
transactions at the minimum relay fee. However due to ECDSA sometimes
making a shorter signature than expected, the size estimate (and
therefore the funded fee) ends up being a little bit too low, which
results in the final transaction being under the min relay fee. We can
compensate for this by just using a feerate higher than the minrelayfee
as the actual feerate itself does not matter in this test.
90c8f79e945863f3818748b86572948d1558aec3 test: remove redundant test values (Jon Atack)
c3f203387df60c596a1e416658d87d68b85adbf2 test: use common assert_signing_completed_successfully helper (Jon Atack)
647d95aae9720543c2c9c46c70191e6f9f775d66 test: add coverage for passing an invalid sighashtype (Jon Atack)
Pull request description:
Add test coverage for passing an invalid sighashtype to RPCs signrawtransactionwithwallet, signrawtransactionwithkey, walletprocesspsbt, and descriptorprocesspsbt.
ACKs for top commit:
MarcoFalke:
lgtm ACK 90c8f79e945863f3818748b86572948d1558aec3 🎥
brunoerg:
light crACK 90c8f79e945863f3818748b86572948d1558aec3
Tree-SHA512: 3861658487edd0d9a377390acf3d43f45c3dd9e324894f0fdb8f5312b618301a55479b1f70c61daee0b20785e768ffde6fa5abe6af190b73c0d0e017f3976704
In `wallet_fundrawtransaction`, `totalOut` is used in
some functions to check if the change is correct. In
other ones, it has been created but never used.
Other tests are also relying on the node1 default wallet,
which thanks to 'test_locked_wallet' is encrypted.
And can only be accessed within a specific timeframe (100ms)
set internally by the same test.
This make other tests susceptible to races. They can only
perform their operations successfully within the specified
time.
This can be seen running the test in valgrind, where other
test cases fail due the wallet re-locking itself after the
100ms.
In functional tests it is a quite common scenario to generate fresh
elliptic curve keypairs, which is currently a bit cumbersome as it
involves multiple steps, e.g.:
privkey = ECKey()
privkey.generate()
privkey_wif = bytes_to_wif(privkey.get_bytes())
pubkey = privkey.get_pubkey().get_bytes()
Simplify this by providing a new `generate_keypair` helper function that
returns the private key either as `ECKey` object or as WIF-string
(depending on the boolean `wif` parameter) and the public key as
byte-string; these formats are what we mostly need (currently we don't
use `ECPubKey` objects from generated keypairs anywhere).
With this, most of the affected code blocks following the pattern above
can be replaced by one-liners, e.g.:
privkey, pubkey = generate_keypair(wif=True)
Note that after this commit, the only direct uses of `ECKey` remain in
situations where we want to set the private key explicitly, e.g. in
MiniWallet (test/functional/test_framework/wallet.py) or the test for
the signet miner script (test/functional/tool_signet_miner.py).
76dc547ee7b05864e7b1b6c55fc0301d47aa3a15 gui: create tx, launch error dialog if backend throws runtime_error (furszy)
f4d79477ff0946b0bd340ade9251fa38e3b95dd7 wallet: coin selection, add duplicated inputs checks (furszy)
0aa065b14e67592d5be8f46ebbe5d59a083ff0a5 wallet: return accurate error messages from Coin Selection (furszy)
7e8340ab1a970a14e180b1fcf420b46a5657b062 wallet: make SelectCoins flow return util::Result (furszy)
e5e147fe97f706e82bc51358f8bdc355f355be57 wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy)
Pull request description:
Work decoupled from #25806, which cleanup and improves the Coin Selection flow further.
Adding the capability to propagate specific error messages from the Coin Selection process to the user.
Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally.
Letting us instruct the user how to proceed under certain circumstances.
The following error messages were added:
1) If the selection result exceeds the maximum transaction weight,
we now will return:
-> "The inputs size exceeds the maximum weight. Please try sending
a smaller amount or manually consolidating your wallet's UTXOs".
2) If the user pre-selected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return:
-> "The preselected coins total amount does not cover the transaction
target. Please allow other inputs to be automatically selected or include
more coins manually".
3) The double-counted preset inputs during Coin Selection error will now
throw an "internal bug detected" message instead of crashing the node.
The essence of this work comes from several comments:
1. https://github.com/bitcoin/bitcoin/pull/26560#discussion_r1037395665
2. https://github.com/bitcoin/bitcoin/pull/25729#discussion_r940619491
3. https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-1135240825
4. https://github.com/bitcoin/bitcoin/issues/23144 (which is connected to #24845)
ACKs for top commit:
ishaanam:
crACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15
achow101:
ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15
aureleoules:
ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15
theStack:
ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15 🌇
Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
and not the general "Insufficient funds" when the wallet
actually have funds.
Two new error messages:
1) If the selection result exceeds the maximum transaction weight,
we now will return: "The inputs size exceeds the maximum weight".
2) If the user preselected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return: "The preselected coins total amount does not cover the
transaction target".
