Commit Graph

380 Commits

Author SHA1 Message Date
am-sq
ca6aa0b9be doc: loadwallet loads from relative walletdir
Improves the documentation of help output for loadwallet
to clarify that filename is relative to the default
wallet directory. Adds examples that get a wallet from
sub-directories.
2025-02-18 15:38:34 -08:00
Ava Chow
0b48f77e10 Merge bitcoin/bitcoin#31413: rpc: Remove deprecated dummy alias for listtransactions::label
fa8e0956c2 rpc: Remove deprecated dummy alias for listtransactions::label (MarcoFalke)

Pull request description:

  The RPC arg is not a dummy, but a label, so offering an undocumented alias is inconsistent with all other label interfaces and confusing at best, if not entirely unused.

  Fix it by removing the deprecated alias.

  This pull is a breaking change, but it should be limited, because it only affects someone using the deprecated named arg on this RPC. I can't imagine anyone doing this, because in all other places where label args are accepted, they are called `label`. If someone really didn't use `label` here as named arg, it would be trivial and less confusing for them to fix it up.

ACKs for top commit:
  achow101:
    ACK fa8e0956c2
  rkrux:
    tACK fa8e0956c2
  ryanofsky:
    Code review ACK fa8e0956c2

Tree-SHA512: 0d0f3f53237ff9fac8c065b7d0a4245f5ff86efa427dbeeca711765494b7315a9d72b44751d346c76422847daf3d7ff90dbccb5ba200b089fb96128bd95da9f0
2025-02-14 13:29:17 -08:00
Ava Chow
85f96b01b7 Merge bitcoin/bitcoin#30909: wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors
9d2d9f7ce2 rpc: Include assumeutxo as a failure reason of rescanblockchain (Fabian Jahr)
595edee169 test, assumeutxo: import descriptors during background sync (Alfonso Roman Zubeldia)
d73ae603d4 rpc: Improve importdescriptor RPC error messages (Fabian Jahr)
27f99b6d63 validation: Don't assume m_chain_tx_count in GuessVerificationProgress (Fabian Jahr)
42d5d53363 interfaces: Add helper function for wallet on pruning (Fabian Jahr)

Pull request description:

  A test that is added as part of #30455 uncovered this issue: The `GuessVerificationProgress` function is used during during descriptor import and relies on `m_chain_tx_count`. In #29370 an [`Assume` was added](0fd915ee6b) expecting the `m_chaint_tx_count` to be set. However, as the test uncovered, `GuessVerificationProgress` is called with background sync blocks that have `m_chaint_tx_count = 0` when they have not been downloaded and processed yet.

  The simple fix is to remove the `Assume`. Users should not be thrown off by the `Internal bug detected` error. The behavior of `importdescriptor` is kept consistent with the behavior for blocks missing due to pruning.

  The test by alfonsoromanz is cherry-picked here to show that the [CI errors](https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535) should be fixed by this change.

  This PR also improves error messages returned by the `importdescriptors` and `rescanblockchain` RPCs. The error message now changes depending on the situation of the node, i.e. if pruning is happening or an assumutxo backgroundsync is active.

ACKs for top commit:
  achow101:
    ACK 9d2d9f7ce2
  mzumsande:
    Code Review ACK 9d2d9f7ce2
  furszy:
    Code review ACK 9d2d9f7ce2

Tree-SHA512: b841a9b371e5eb8eb3bfebca35645ff2fdded7a3e5e06308d46a33a51ca42cc4c258028c9958fbbb6cda9bb990e07ab8d8504dd9ec6705ef78afe0435912b365
2025-01-31 15:45:14 -05:00
Ava Chow
c7869cb214 Merge bitcoin/bitcoin#30844: RPC: improve SFFO arg parsing, error catching and coverage
cddcbaf81e RPC: improve SFFO arg parsing, error catching and coverage (furszy)
4f4cd35319 rpc: decouple sendtoaddress 'subtractfeefromamount' boolean parsing (furszy)

Pull request description:

  Following changes were made:

  1) Catch and signal error for duplicate string destinations.
  2) Catch and signal error for invalid value type.
  3) Catch and signal error for string destination not found in tx outputs.
  4) Improved `InterpretSubtractFeeFromOutputInstructions()` code organization.
  5) Added test coverage for all possible error failures.

  Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file:
  - PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class declaration.
  - PEP 8: E303 too many blank lines (2) at skip_test_if_missing_module() and set_test_params()

ACKs for top commit:
  achow101:
    ACK cddcbaf81e
  murchandamus:
    crACK cddcbaf81e
  naiyoma:
    TACK [cddcbaf81e)
  ismaelsadeeq:
    Code review and Tested ACK cddcbaf81e

Tree-SHA512: c9c15582b81101a93987458d155394ff2c9ca42864624c034ee808a31c3a7d7f55105dea98e86fce17d3c7b2c1a6b5b77942da66b287f8b8881a60cde78c1a3c
2025-01-29 16:33:13 -05:00
merge-script
d5a2ba44ba Merge bitcoin/bitcoin#31416: doc: Fix incorrect send RPC docs
fad83e759a doc: Fix incorrect send RPC docs (MarcoFalke)

Pull request description:

  It would be good to have accurate RPC docs, so that humans and machines can read them and rely on them.

  This fixes one issue.

ACKs for top commit:
  fjahr:
    utACK fad83e759a
  rkrux:
    tACK fad83e759a
  luke-jr:
    tACK fad83e759a

Tree-SHA512: 65d0cc18a62ef44833621464d74b743d24ffe2b853596dce2c4f423df0495142d50387c02ba1b54f5ca77d4ddb083d55116a8ac92698aa6558762d841664911e
2025-01-23 17:42:53 +00:00
MarcoFalke
eeee6cf2ff refactor: Delay translation of _() literals
This is required for a future commit that requires _() to be consteval
for format literals.

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
2025-01-14 19:21:37 +01:00
Fabian Jahr
9d2d9f7ce2 rpc: Include assumeutxo as a failure reason of rescanblockchain 2025-01-05 17:28:34 +01:00
Fabian Jahr
d73ae603d4 rpc: Improve importdescriptor RPC error messages
Particularly add more details in the case of pruning or assumeutxo.
2025-01-05 17:28:34 +01:00
MarcoFalke
fad83e759a doc: Fix incorrect send RPC docs 2024-12-04 15:52:46 +01:00
MarcoFalke
fa8e0956c2 rpc: Remove deprecated dummy alias for listtransactions::label 2024-12-03 16:53:37 +01:00
MarcoFalke
faf70cc994 Remove wallet::ParseISO8601DateTime, use ParseISO8601DateTime instead 2024-12-02 15:09:31 +01:00
MarcoFalke
fa1177e3d7 refactor: Avoid std::string format strings
Pass literal format strings instead of std::string so formats can be
checked at compile time.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
2024-11-14 12:44:13 +01:00
Hennadii Stepanov
70713303b6 scripted-diff: Rename PACKAGE_* variables to CLIENT_*
This change ensures consistent use of the `CLIENT_` namespace everywhere
in the repository.

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./cmake ./src :\(exclude\)./src/secp256k1 ./test ) ; }

ren PACKAGE_NAME      CLIENT_NAME
ren PACKAGE_VERSION   CLIENT_VERSION_STRING
ren PACKAGE_URL       CLIENT_URL
ren PACKAGE_BUGREPORT CLIENT_BUGREPORT

-END VERIFY SCRIPT-
2024-10-28 12:36:19 +00:00
Sebastian Falbesoner
1786be7b4a scripted-diff: drop config/ subdir for bitcoin-config.h, rename to bitcoin-build-config.h
Follow-up for PR #30856, commit 0dd66251.

-BEGIN VERIFY SCRIPT-
sed -i "s|config/bitcoin-config\.h|bitcoin-build-config.h|g" $(git grep -l config/bitcoin-config\.h)
sed -i "s|bitcoin-config\.h|bitcoin-build-config.h|g" $(git grep -l "bitcoin-config\.h" ./src ./test ./cmake)
git mv ./cmake/bitcoin-config.h.in ./cmake/bitcoin-build-config.h.in
-END VERIFY SCRIPT-
2024-10-10 12:22:12 +02:00
pablomartin4btc
54227e681a rpc, cli: improve error message on multiwallet mode
The primary objective is to provide users with clearer
and more informative error messages when encountering
the RPC_WALLET_NOT_SPECIFIED error, which occurs when
multiple wallets are loadad.

This commit also rectifies the error message consistency
by bringing the error message in line with the definition
established in protocol.h ("error when there are multiple
wallets loaded").
2024-09-17 16:22:12 -03:00
furszy
cddcbaf81e RPC: improve SFFO arg parsing, error catching and coverage
Following changes were made:

1) Catch and signal error for duplicate string destinations.
2) Catch and signal error for invalid value type.
3) Catch and signal error for string destination not found in tx outputs.
4) Improved 'InterpretSubtractFeeFromOutputInstructions()' code organization.
5) Added test coverage for all possible error failures.

Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file:
- PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class declaration.
- PEP 8: E303 too many blank lines (2) at skip_test_if_missing_module() and set_test_params()
2024-09-07 13:06:41 -03:00
furszy
4f4cd35319 rpc: decouple sendtoaddress 'subtractfeefromamount' boolean parsing
The 'subtractfeefromamount' arg is only boolean for sendtoaddress().
Other commands should never provide it as a boolean.
2024-09-07 12:15:46 -03:00
glozow
f93d5553d1 Merge bitcoin/bitcoin#22838: descriptors: Be able to specify change and receiving in a single descriptor string
a0abcbd382 doc: Mention multipath specifier (Ava Chow)
0019f61fc5 tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4b wallet: Move internal to be per key when importing (Ava Chow)
1692245525 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9 rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd22 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2da descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02 descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b1034 descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae15 descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f72 descriptors: Add PubkeyProvider::Clone (Ava Chow)

Pull request description:

  It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.

  To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.

  This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.

  The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).

  Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.

  Closes #17190

ACKs for top commit:
  darosior:
    re-ACK a0abcbd382
  mjdietzx:
    reACK a0abcbd382
  pythcoiner:
    reACK a0abcbd
  furszy:
    Code review ACK a0abcbd
  glozow:
    light code review ACK a0abcbd382

Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
2024-08-28 15:56:15 +01:00
Ava Chow
99ecb9a630 Merge bitcoin/bitcoin#30659: wallet: fix UnloadWallet thread safety assumptions
f550a8e035 Rename ReleaseWallet to FlushAndDeleteWallet (furszy)
64e736d79e wallet: WaitForDeleteWallet, do not expect thread safety (Ryan Ofsky)
8872b4a6ca wallet: rename UnloadWallet to WaitForDeleteWallet (furszy)
5d15485aaf wallet: unload, notify GUI as soon as possible (furszy)

Pull request description:

  Coming from #29073.

  Applied ryanofsky suggested changes on https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242 with few modifications coming from https://github.com/bitcoin/bitcoin/pull/18338#issuecomment-605060348.

  The only point I did not tackle from https://github.com/bitcoin/bitcoin/pull/18338#issuecomment-605060348 is:

  > * Move log print and flush out of ReleaseWallet into CWallet destructor

  Because it would mean every `CWallet` object would flush data to disk during destruction. Which is not necessary for wallet tool utilities and unit tests.

ACKs for top commit:
  achow101:
    ACK f550a8e035
  ryanofsky:
    Code review ACK f550a8e035. Just a simple rename since last review
  ismaelsadeeq:
    Re-ACK f550a8e035

Tree-SHA512: e2eb69bf36883c514f601f4838ae6a41113996b9559abf8dc2b46e16bbcdad401195ac0f2b9d1fb55a10e78bb8ea9953788a168c80474e3f101350d208cb3bd2
2024-08-15 13:22:34 -04:00
furszy
8872b4a6ca wallet: rename UnloadWallet to WaitForDeleteWallet
And update function's documentation.
2024-08-14 16:12:18 -03:00
Ava Chow
28fc562f26 wallet, interfaces: Include database format in listWalletDir 2024-08-13 11:25:38 -04:00
merge-script
190033600b Merge bitcoin/bitcoin#30524: doc: rpc: Use "output script" consistently (2/2)
fa5755b0a8 doc: rpc: Use "output script" consistently (2/2) (MarcoFalke)

Pull request description:

  Small follow-up to https://github.com/bitcoin/bitcoin/pull/30408 to fixup the RPCs that were forgotten.

ACKs for top commit:
  theStack:
    lgtm ACK fa5755b0a8

Tree-SHA512: f1fc0aabb59017da216d6fe0f08a2274336d04db332ad6ce3d9608cd6f03667be1c76423f24a489ac8e7d536011a129dca752ab64b4621b7bc1d4d53f68602e4
2024-08-12 11:19:13 +01:00
Ava Chow
f97d5c137d wallet, rpc: Allow importdescriptors to import multipath descriptors
Multipath descriptors will be imported as multiple separate descriptors.
When there are 2 multipath items, the first descriptor will be for receiving
addresses and the second for change. This mirrors importmulti.
2024-08-08 12:47:38 -04:00
Ava Chow
32dcbca3fb rpc: Allow importmulti to import multipath descriptors correctly
Multipath descriptors will be imported as multiple separate descriptors.
When there are exactly 2 multipath items, the first descriptor will be
for receiving addreses, and the second for change
addresses. When importing a multipath descriptor, 'internal' cannot be
specified.
2024-08-08 12:47:38 -04:00
Ava Chow
64dfe3ce4b wallet: Move internal to be per key when importing
Instead of applying internal-ness to all keys being imported at the same
time, apply it on a per key basis. So each key that is imported will
carry with it whether it is for the change keypool.
2024-08-08 12:47:38 -04:00
Ava Chow
1bbf46e2da descriptors: Change Parse to return vector of descriptors
When given a descriptor which contins a multipath derivation specifier,
a vector of descriptors will be returned.
2024-08-08 12:47:22 -04:00
Ava Chow
2987ba6637 Merge bitcoin/bitcoin#30525: doc, rpc : #30275 followups
fa2f26960e [rpc, fees]: add more detail on the fee estimation modes (ismaelsadeeq)
6e7e620864 [doc]: add `30275` release notes (ismaelsadeeq)

Pull request description:

  This PR:
  1. Adds release notes for #30275
  2. Describe fee estimation modes in RPC help texts

ACKs for top commit:
  achow101:
    ACK fa2f26960e
  glozow:
    ACK fa2f26960e
  willcl-ark:
    ACK fa2f26960e
  tdb3:
    re ACK fa2f26960e

Tree-SHA512: b8ea000b599297b954dc770137c29b47153e68644c58550a73e34b74ecb8b65e78417875481efdfdf6aab0018a9cd1d90d8baa5a015e70aca0975f6e1dc9598c
2024-08-07 01:27:42 -04:00
ismaelsadeeq
fa2f26960e [rpc, fees]: add more detail on the fee estimation modes
- Add description that indicates the fee estimation modes behaviour.
- This description will be returned in the RPC's help texts.
2024-08-02 15:40:43 +01:00
Greg Sanders
455fca86cf policy: Add OP_1 <0x4e73> as a standard output type
These outputs are called anchors, and allow
key-less anchor spends which are vsize-minimized
versus keyed anchors which require larger outputs
when creating and inputs when spending.
2024-07-30 14:06:58 -04:00
MarcoFalke
fa5755b0a8 doc: rpc: Use "output script" consistently (2/2) 2024-07-25 16:36:08 +02:00
Ava Chow
efbf4e71ce Merge bitcoin/bitcoin#29523: Wallet: Add max_tx_weight to transaction funding options (take 2)
734076c6de [wallet, rpc]: add `max_tx_weight` to tx funding options (ismaelsadeeq)
b6fc5043c1 [wallet]: update the data type of `change_output_size`, `change_spend_size` and `tx_noinputs_size` to `int` (ismaelsadeeq)
baab0d2d43 [doc]: update reason for deducting change output weight (ismaelsadeeq)
7f61d31a5c [refactor]: update coin selection algorithms input parameter `max_weight` name (ismaelsadeeq)

Pull request description:

  This PR taken over from #29264

  The PR added an option `max_tx_weight` to transaction funding RPC's that ensures the resulting transaction weight does not exceed the specified `max_tx_weight` limit.

  If `max_tx_weight` is not given `MAX_STANDARD_TX_WEIGHT` is used as the max threshold.

  This PR addressed outstanding review comments in #29264

  For more context and rationale behind this PR see https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs

ACKs for top commit:
  achow101:
    ACK 734076c6de
  furszy:
    utACK 734076c6de
  rkrux:
    reACK [734076c](734076c6de)

Tree-SHA512: 013501aa443d239ee2ac01bccfc5296490c27b4edebe5cfca6b96c842375e895e5cfeb5424e82e359be581460f8be92095855763a62779a18ccd5bdfdd7ddce7
2024-07-17 18:27:59 -04:00
MarcoFalke
fa6390df20 doc: getaddressinfo[isscript] is optional 2024-07-17 06:51:58 +02:00
ishaanam
7d55796c53 wallet: update mempool conflicts tests + docs 2024-07-01 12:27:43 -04:00
ismaelsadeeq
734076c6de [wallet, rpc]: add max_tx_weight to tx funding options
This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.

Co-authored-by: Greg Sanders <gsanders87@gmail.com>
2024-06-27 15:31:21 +01:00
Cory Fields
5729dbbb74 refactor: remove extraneous lock annotations from function definitions
These annotations belong in the declarations rather than the definitions.
While harmless now, future versions of clang may warn about these.
2024-06-20 18:45:32 +00:00
Ava Chow
011a895a82 Merge bitcoin/bitcoin#29015: kernel: Streamline util library
c7376babd1 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334 util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b4 util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9a common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae3 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608d util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad4792 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff4 util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420c build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24d test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)

Pull request description:

  Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:

  - Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
  - Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
  - Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.

  Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.

ACKs for top commit:
  achow101:
    ACK c7376babd1
  TheCharlatan:
    Re-ACK c7376babd1
  hebasto:
    re-ACK c7376babd1.

Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
2024-06-12 17:12:54 -04:00
Ava Chow
76a33be21d Merge bitcoin/bitcoin#28307: rpc, wallet: fix incorrect segwit redeem script size limit
2451a217dd test: addmultisigaddress, coverage for script size limits (furszy)
53302a0981 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes (furszy)
9be6065cc0 test: coverage for 16-20 segwit multisig scripts (furszy)
9d9a91c4ea rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey (furszy)
0c9fedfc45 fix incorrect multisig redeem script size limit for segwit (furszy)
f7a173b578 test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' (furszy)
4f33dbd8f8 test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' (furszy)
25a81705d3 test: rpc_createmultisig, remove unnecessary checkbalances() (furszy)
b5a3289433 test: refactor, multiple cleanups in rpc_createmultisig.py (furszy)
3635d43268 test: rpc_createmultisig, remove manual wallet initialization (furszy)

Pull request description:

  Fixing https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674830104 and more.

  Currently, redeem scripts longer than 520 bytes, which are technically valid under segwit rules, have flaws in the following processes:
  1) The multisig creation process fails to deduce the output descriptor, resulting in the generation of an incorrect descriptor. Additionally, the accompanying user warning is also inaccurate.
  2) The `signrawtransactionwithkey` RPC command fail to sign them.
  3) The legacy wallet `addmultisigaddress` wrongly discards them.

  The issue arises because most of these flows are utilizing the legacy spkm keystore, which imposes
  the [p2sh max redeem script size rule](ded6873340/src/script/signingprovider.cpp (L160)) on all scripts. Which blocks segwit redeem scripts longer than
  the max element size in all the previously mentioned processes (`createmultisig`, `addmultisigaddress`, and
  `signrawtransactionwithkey`).

  This PR fixes the problem, enabling the creation of multisig output descriptors involving more than 15 keys and
  allowing the signing of these scripts, along with other post-segwit redeem scripts that surpass the 520-byte
  p2sh limit.

  Important note:
  Instead of adding support for these longer redeem scripts in the legacy wallet, an "unsupported operation"
  error has been added. The reasons behind this decision are:

  1) The introduction of this feature brings about a compatibility-breaking change that requires downgrade
      protection; older wallets would be unable to interact with these "new" legacy wallets.

  2) Considering the ongoing deprecation of the legacy spkm, this issue provides another compelling
      reason to transition towards descriptors.

  Testing notes:
  To easily verify each of the fixes, I decoupled the tests into standalone commits. So they can be
  cherry-picked on top of master. Where `rpc_createmultisig.py` (with and without the `--legacy-wallet`
  arg) will fail without the bugs fixes commits.

  Extra note:
  The initial commits improves the `rpc_createmultisig.py` test in many ways. I found this test very
  antiquated, screaming for an update and cleanup.

ACKs for top commit:
  pinheadmz:
    ACK 2451a217dd
  theStack:
    Code-review ACK 2451a217dd
  achow101:
    ACK 2451a217dd

Tree-SHA512: 71794533cbd46b3a1079fb4e9d190d3ea3b615de0cbfa443466e14f05e4616ca90e12ce2bf07113515ea8113e64a560ad572bb9ea9d4835b6fb67b6ae596167f
2024-06-04 21:39:49 -04:00
Ava Chow
09fe1435d9 Merge bitcoin/bitcoin#29997: rpc: Remove index-based Arg accessor
fa3169b073 rpc: Remove index-based Arg accessor (MarcoFalke)

Pull request description:

  The index-based Arg accessor is redundant with the name-based one. It does not provide any benefit to the code reader, or otherwise, so remove it.

ACKs for top commit:
  stickies-v:
    re-ACK fa3169b073, addressed doc nits
  achow101:
    ACK fa3169b073
  ryanofsky:
    Code review ACK fa3169b073. One changes since last review are some documentation improvements

Tree-SHA512: f9da1c049dbf38c3b47a8caf8d24d195c2d4b88c7ec45a9ccfb78f1e39f29cb86869f84b308f6e49856b074c06604ab634c90eb89c9c93d2a8169e070aa1bd40
2024-06-04 20:11:59 -04:00
Ava Chow
e54c392356 Merge bitcoin/bitcoin#28979: wallet, rpc: document and update sendall behavior around unconfirmed inputs
71aae72e1f test: test sendall does ancestor aware funding (ishaanam)
36757941a0 wallet, rpc: implement ancestor aware funding for sendall (ishaanam)
544131f3fb rpc, test: test sendall spends unconfirmed change and unconfirmed inputs when specified (ishaanam)

Pull request description:

  This PR:
  - Adds a functional test that `sendall` spends unconfirmed change
  - Adds a functional test that `sendall` spends regular unconfirmed inputs when specified by user
  - Adds ancestor aware funding to `sendall` by using `calculateCombinedBumpFee` and adjusting the effective value accordingly
  - Adds a functional test for ancestor aware funding in `sendall`

ACKs for top commit:
  S3RK:
    ACK 71aae72e1f
  achow101:
    ACK 71aae72e1f
  furszy:
    ACK 71aae72e1f

Tree-SHA512: acaeb7c65166ce53123a1d6cb5012197202246acc02ef9f37a28154cc93afdbd77c25e840ab79bdc7e0b88904014a43ab1ddea79d5337dc310ea210634ab61f0
2024-06-04 18:46:47 -04:00
Cory Fields
d7707d9843 rpc: avoid copying into UniValue
These are simple (and hopefully obviously correct) copies that can be moves
instead.
2024-05-20 16:48:19 +00:00
Ryan Ofsky
4f74c59334 util: Move util/string.h functions to util namespace
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.

Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
2024-05-16 10:16:08 -05:00
Ryan Ofsky
4d05d3f3b4 util: add TransactionError includes and namespace declarations
Add TransactionError to node namespace and include it directly instead of
relying on indirect include through common/messages.h

This is a followup to a previous commit which moved the TransactionError enum.
These changes were done in a separate followup just to keep the previous commit
more minimal and easy to review.
2024-05-16 10:16:08 -05:00
Ryan Ofsky
680eafdc74 util: move fees.h and error.h to common/messages.h
Move enum and message formatting functions to a common/messages header where
they should be more discoverable, and also out of the util library, so they
will not be a dependency of the kernel

The are no changes in behavior and no changes to the moved code.
2024-05-16 10:16:08 -05:00
Ryan Ofsky
02e62c6c9a common: Add PSBTError enum
Add separate PSBTError enum instead of reusing TransactionError enum for PSBT
operations, and drop unused error codes. The error codes returned by PSBT
operations and transaction broadcast functions mostly do not overlap, so using
an unified enum makes it harder to call any of these functions and know which
errors actually need to be handled.

Define PSBTError in the common library because PSBT functionality is
implemented in the common library and used by both the node (for rawtransaction
RPCs) and the wallet.
2024-05-16 10:16:08 -05:00
Ryan Ofsky
6861f954f8 util: move util/message to common/signmessage
Move util/message to common/signmessage so it is named more clearly, and
because the util library is not supposed to depend on other libraries besides
the crypto library. The signmessage functions use CKey, CPubKey, PKHash, and
DecodeDestination functions in the consensus and common libraries.
2024-05-16 11:16:08 -04:00
MarcoFalke
fa3169b073 rpc: Remove index-based Arg accessor 2024-05-15 17:21:14 +02:00
Ava Chow
4ff42762fd Merge bitcoin/bitcoin#28336: rpc: parse legacy pubkeys consistently with specific error messages
98570fe29b test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d1 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf 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 98570fe29b

Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
2024-05-08 17:52:58 -04:00
furszy
53302a0981 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes
Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed
on 'p2sh-segwit' and 'bech32' redeem scripts.
Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for
segwit output types, we don't want to enable this feature in legacy wallets for the
following reasons:

1) It introduces a compatibility-breaking change requiring downgrade protection; older
   wallets would be unable to interact with these "new" legacy wallets.

2) Considering the ongoing deprecation of the legacy spkm, this issue adds another
   good reason to transition towards descriptors.
2024-05-03 14:20:45 -03:00
furszy
0c9fedfc45 fix incorrect multisig redeem script size limit for segwit
The multisig script generation process currently fails when
the user exceeds 15 keys, even when it shouldn't. The maximum
number of keys allowed for segwit redeem scripts (p2sh-segwit
and bech32) is 20 keys.
This is because the redeem script placed in the witness is not
restricted by the item size limit.

The reason behind this issue is the utilization of the legacy
p2sh redeem script restrictions on segwit ones. Redeem scripts
longer than 520 bytes are blocked from being inserted into the
keystore, which causes the signing process and the descriptor
inference process to fail.

This occurs because the multisig generation flow uses the same
keystore as the legacy spkm (FillableSigningProvider), which
contains the 520-byte limit.
2024-05-03 14:20:44 -03:00
MarcoFalke
dddd40ba82 scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes
-BEGIN VERIFY SCRIPT-
 perl -0777 -pi -e 's/#if defined\(HAVE_CONFIG_H\)\n#include <config\/bitcoin-config.h>.*\n#endif.*\n/#include <config\/bitcoin-config.h> \/\/ IWYU pragma: keep\n/g' $( git grep -l '#include <config/bitcoin-config.h>' )
-END VERIFY SCRIPT-
2024-05-01 08:33:04 +02:00