aboutsummaryrefslogtreecommitdiff
path: root/src/wallet
AgeCommit message (Collapse)Author
2022-09-19Merge bitcoin/bitcoin#26005: Wallet: Fix error handling (copy_file failure ↵fanquake
in RestoreWallet, and in general via interfaces) c3e536555aa3a7db773170671da1256a2ace2094 Bugfix: Wallet: Return util::Error rather than non-error nullptr when CreateWallet/LoadWallet/RestoreWallet fail (Luke Dashjr) 335ff98c8a64eda38a2a2334102bd253f108c253 Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure exceptions become returned errors and incomplete wallet directory is removed (Luke Dashjr) Pull request description: Bug 1: `copy_file` can throw exceptions, but `RestoreWallet` is expected to return a nullptr with a populated `errors` parameter. This is fixed by wrapping `copy_file` and `LoadWallet` (for good measure) in a `try` block, and converting any exceptions to the intended return style. Bug 2: `util::Result` turns what would have been a `false` unique_ptr into a `true` nullptr result, which leads to nullptr dereferences in at least the 3 cases of wallet creation/loading/restoring. This is fixed by keeping the pointer as a plain `std::unique_ptr` until actually returning it (ie, after the nullptr check). Fixes https://github.com/bitcoin-core/gui/issues/661 ACKs for top commit: achow101: ACK c3e536555aa3a7db773170671da1256a2ace2094 Tree-SHA512: 4291b3dbbb147acea2e63a704324c9371bc16ecb4237f8753729b0b0a6e55c9758ad61bfe8bd432fd7b0bae95d8b63a9831e61ac8b8d5c0197b550a2e0f4a105
2022-09-16Bugfix: Wallet: Return util::Error rather than non-error nullptr when ↵Luke Dashjr
CreateWallet/LoadWallet/RestoreWallet fail
2022-09-16Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure ↵Luke Dashjr
exceptions become returned errors and incomplete wallet directory is removed
2022-09-16Merge bitcoin/bitcoin#25499: Use steady clock for all millis bench loggingfanquake
fa521c960337a65d4ce12cd1ef009c652ffe57e6 Use steady clock for all millis bench logging (MacroFake) Pull request description: Currently `GetTimeMillis` is used for bench logging in milliseconds integral precision. Replace it to use a steady clock that is type-safe and steady. Microsecond or float precision can be done in a follow-up. ACKs for top commit: fanquake: ACK fa521c960337a65d4ce12cd1ef009c652ffe57e6 - started making the same change. Tree-SHA512: 86a810e496fc663f815acb8771a6c770331593715cde85370226685bc50c13e8e987e3c5efd0b4e48b36ebd2372255357b709204bac750d41e94a9f7d9897fa6
2022-09-15Merge bitcoin/bitcoin#26024: wallet: fix sendall creates tx that fails ↵Andrew Chow
tx-size check cc434cbf583ec8d1b0f3aa504417231fdc81f33a wallet: fix sendall creates tx that fails tx-size check (kouloumos) Pull request description: Fixes https://github.com/bitcoin/bitcoin/issues/26011 The `sendall` RPC doesn't use `CreateTransactionInternal` as the rest of the wallet RPCs. [This has already been discussed in the original PR](https://github.com/bitcoin/bitcoin/pull/24118#issuecomment-1029462114). By not going through that path, it never checks the transaction's weight against the maximum tx weight for transactions we're willing to relay. https://github.com/bitcoin/bitcoin/blob/447f50e4aed9a8b1d80e1891cda85801aeb80b4e/src/wallet/spend.cpp#L1013-L1018 This PR adds a check for tx-size as well as test coverage for that case. _Note: It seems that the test takes a bit of time on slower machines, I'm not sure if dropping it might be for the better._ ACKs for top commit: glozow: re ACK cc434cb via range-diff. Changes were addressing https://github.com/bitcoin/bitcoin/pull/26024#discussion_r971325299 and https://github.com/bitcoin/bitcoin/pull/26024#discussion_r970651614. achow101: ACK cc434cbf583ec8d1b0f3aa504417231fdc81f33a w0xlt: reACK https://github.com/bitcoin/bitcoin/pull/26024/commits/cc434cbf583ec8d1b0f3aa504417231fdc81f33a Tree-SHA512: 64a1d8f2c737b39f3ccee90689eda1dd9c1b65f11b2c7bc0ec8bfe72f0202ce90ab4033bb0ecfc6080af8c947575059588a00938fe48e7fd553f7fb5ee03b3cc
2022-09-15Merge bitcoin/bitcoin#26091: test: Fix syncwithvalidationinterfacequeue callsAndrew Chow
fa1ce96184a1815f453e64e14d77cb0025800be9 test: Add missing syncwithvalidationinterfacequeue (MacroFake) faa4916529699f9a057e2bf2459d957bcec1de84 test/doc: Remove unused syncwithvalidationinterfacequeue (MacroFake) Pull request description: Fixes #26071 ACKs for top commit: achow101: ACK fa1ce96184a1815f453e64e14d77cb0025800be9 glozow: ACK fa1ce96184a1815f453e64e14d77cb0025800be9 w0xlt: ACK https://github.com/bitcoin/bitcoin/commit/fa1ce96184a1815f453e64e14d77cb0025800be9 Tree-SHA512: d1e101b55477360ead2b99ade5d42b922aabe293ec84fb26764e29161c5be6c534aef6f22d2cc5ea63a4bd6b6e77b701f1a7a2283b8e7e815d343a604cd77656
2022-09-15wallet: fix sendall creates tx that fails tx-size checkkouloumos
The `sendall` RPC doesn't use `CreateTransactionInternal`as the rest of the wallet RPCs and it never checks against the tx-size mempool limit. Add a check for tx-size as well as test coverage for that case.
2022-09-15test: Add missing syncwithvalidationinterfacequeueMacroFake
2022-09-15Merge bitcoin/bitcoin#26084: sendall: check if the maxtxfee has been exceededMacroFake
6f8e3818af7585b961039bf0c768be2e4ee44e0f sendall: check if the maxtxfee has been exceeded (ishaanam) Pull request description: Previously the `sendall` RPC didn't check whether the fees of the transaction it creates exceed the set `maxtxfee`. This PR adds this check to `sendall` and a test case for it. ACKs for top commit: achow101: ACK 6f8e3818af7585b961039bf0c768be2e4ee44e0f Xekyo: ACK 6f8e3818af7585b961039bf0c768be2e4ee44e0f glozow: Concept ACK 6f8e3818af7585b961039bf0c768be2e4ee44e0f. The high feerate is unlikely but sendall should respect the existing wallet options. Tree-SHA512: 6ef0961937091293d49be16f17e4451cff3159d901c0c7c6e508883999dfe0c20ed4d7126bf74bfea8150d4c1eef961a45f0c28ef64562e6cb817fede2319f1a
2022-09-14Merge bitcoin/bitcoin#26053: rpc: bugfix, 'add_inputs' default value is true ↵Andrew Chow
unless 'inputs' are provided b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f test: add coverage for 'add_inputs' dynamic default value (furszy) ddbcfdf3d050061f4e8979a0e2bb63bba5a43c47 RPC: bugfix, 'add_inputs' default value is true unless 'inputs' are provided (furszy) Pull request description: This bugfix was meant to be in #25685, but decoupled it to try to make it part of 24.0 release. It's a truly misleading functionality. This PR doesn't change behavior in any way. Just fixes two invalid RPC help messages and adds test coverage for the current behavior. #### Description In both RPC commands `send()` and `walletcreatefundedpsbt` the help message says that `add_inputs` default value is false when it's actually dynamically set by the following statement: ```c++ coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; ``` Which means that, by default, `add_inputs` is true unless there is any pre-set input, in which case, the default is false. ACKs for top commit: achow101: ACK b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f S3RK: ACK b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f Tree-SHA512: 5c68a40d81c994e0ab6de0817db69c4d3dea3a9a64a60362531bf583b7a4c37d524b740905a3f3a89cdbf221913ff5b504746625adb8622788aea93a35bbcd40
2022-09-14test/doc: Remove unused syncwithvalidationinterfacequeueMacroFake
See https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958562071 Also fix doc typo from https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958571943
2022-09-13sendall: check if the maxtxfee has been exceededishaanam
2022-09-13RPC: bugfix, 'add_inputs' default value is true unless 'inputs' are providedfurszy
In both RPC commands `send()` and `walletcreatefundedpsbt` the RPC help was saying that `add_inputs` default value was false when it's actually dynamically set by the following statement: `coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;` Which means that, by default, `add_inputs` is true unless there was any pre-set input, in which case, the default is false.
2022-09-13Merge bitcoin/bitcoin#26021: wallet: bugfix, load a wallet with an ↵Andrew Chow
unknown/corrupt descriptor causes a fatal error e06676377d935c69f0ee51fc18eb0772d524aba5 wallet: coverage for loading an unknown descriptor (furszy) d26c3cc44438ecb9e4f618a2427c3c92a292aa16 wallet: bugfix, load wallet with an unknown descriptor cause fatal error (furszy) Pull request description: Fixes #26015 If the descriptor entry is unrecognized (due a soft downgrade) or corrupt, the unserialization fails and `LoadWallet`, instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized or corrupt descriptor are scanned, a fatal error is being thrown. This fixes it by catching the descriptor parse failure and return which wallet failed. Logging its name/path, so the user can remove it from the settings file, to prevent its load at startup. Note: added the test in a separate file intentionally. Will continue adding coverage for the wallet load process in follow-up PRs. ACKs for top commit: achow101: ACK e06676377d935c69f0ee51fc18eb0772d524aba5 Sjors: re-utACK e06676377d935c69f0ee51fc18eb0772d524aba5 Tree-SHA512: d1f1a5d7e944c89c97a33b25b4411a36a11edae172c22f8524f69c84a035f84c570b284679f901fe60f1300f781b76a6c17b015a8e7ad44ebd25a0c295ef260f
2022-09-09wallet: coverage for loading an unknown descriptorfurszy
Previously, this was crashing the wallet.
2022-09-09wallet: bugfix, load wallet with an unknown descriptor cause fatal errorfurszy
If the descriptor entry is unrecognized/corrupt, the unserialization fails and `LoadWallet` instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized/corrupted descriptor are scanned, a fatal error is thrown.
2022-09-06Merge bitcoin/bitcoin#26010: RPC: fix sendall docsAndrew Chow
51829409967dcfd039249506ecd2c58fb9a74b2f RPC: fix sendall docs (Anthony Towns) Pull request description: Updates the documentation for the "inputs" entry in the "options" parameter of the sendall RPC to match the documentation for createrawtransaction. ACKs for top commit: achow101: ACK 51829409967dcfd039249506ecd2c58fb9a74b2f Xekyo: ACK 51829409967dcfd039249506ecd2c58fb9a74b2f Tree-SHA512: fe78e17b2f36190939b645d7f4653d025bbac110e4a7285b49e7f1da27adac8c4d03fd5b770e3a74351066b1ab87fde36fc796f42b03897e4e2ebef4b6b6081c
2022-09-05RPC: fix sendall docsAnthony Towns
Updates the documentation for the "inputs" entry in the "options" parameter of the sendall RPC to match the documentation for createrawtransaction.
2022-09-05Merge bitcoin/bitcoin#25768: wallet: Properly rebroadcast unconfirmed ↵glozow
transaction chains 3405f3eed5cf841b23a569b64a376c2e5b5026cd test: Test that an unconfirmed not-in-mempool chain is rebroadcast (Andrew Chow) 10d91c5abe9ed7dcc237c9d52c588e7d26e162a4 wallet: Deduplicate Resend and ReacceptWalletTransactions (Andrew Chow) Pull request description: Currently `ResendWalletTransactions` (used for normal rebroadcasts) will attempt to rebroadcast all of the transactions in the wallet in the order they are stored in `mapWallet`. This ends up being random as `mapWallet` is a `std::unordered_map`. However `ReacceptWalletTransactions` (used for adding to the mempool on loading) first sorts the txs by wallet insertion order, then submits them. The result is that `ResendWalletTranactions` will fail to rebroadcast child transactions if their txids happen to be lexicographically less than their parent's txid. This PR resolves this issue by combining `ReacceptWalletTransactions` and `ResendWalletTransactions` into a new `ResubmitWalletTransactions` so that the iteration code and basic checks are shared. A test has also been added that checks that such transaction chains are rebroadcast correctly. ACKs for top commit: naumenkogs: utACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd 1440000bytes: reACK https://github.com/bitcoin/bitcoin/pull/25768/commits/3405f3eed5cf841b23a569b64a376c2e5b5026cd furszy: Late code review ACK 3405f3ee stickies-v: ACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd Tree-SHA512: 1240d9690ecc2ae8d476286b79e2386f537a90c41dd2b8b8a5a9c2a917aa3af85d6aee019fbbb05e772985a2b197e2788305586d9d5dac78ccba1ee5aa31d77a
2022-09-01Merge bitcoin/bitcoin#19602: wallet: Migrate legacy wallets to descriptor ↵Andrew Chow
wallets 53e7ed075c49f853cc845afc7b2f058cabad0cb0 doc: Release notes and other docs for migration (Andrew Chow) 9c44bfe244f35f08ba576d8b979a90dcd68d2c77 Test migratewallet (Andrew Chow) 0b26e7cdf2659fd8b54d21fd2bd749f9f3e87af8 descriptors: addr() and raw() should return false for ToPrivateString (Andrew Chow) 31764c3f872f4f01b48d50585f86e97c41554954 Add migratewallet RPC (Andrew Chow) 0bf7b38bff422e7413bcd3dc0abe2568dd918ddc Implement MigrateLegacyToDescriptor (Andrew Chow) e7b16f925ae5b117e8b74ce814b63e19b19b50f4 Implement MigrateToSQLite (Andrew Chow) 5b62f095e790a0d4e2a70ece89465b64fc68358a wallet: Refactor SetupDescSPKMs to take CExtKey (Andrew Chow) 22401f17e026ead4bc3fe96967eec56a719a4f75 Implement LegacyScriptPubKeyMan::DeleteRecords (Andrew Chow) 35f428fae68ad974abdce0fa905148f620a9443c Implement LegacyScriptPubKeyMan::MigrateToDescriptor (Andrew Chow) ea1ab390e4dac128e3a37d4884528c3f4128ed83 scriptpubkeyman: Implement GetScriptPubKeys in Legacy (Andrew Chow) e664af29760527e75cd7e290be5f102b6d29ebee Apply label to all scriptPubKeys of imported combo() (Andrew Chow) Pull request description: This PR adds a new `migratewallet` RPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching from `getnewaddress` or `getrawchangeaddress`. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any active `ScriptPubKeyMan`s. For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active `ScriptPubKeyMan`s. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor. There are also tests. ACKs for top commit: Sjors: tACK 53e7ed075c49f853cc845afc7b2f058cabad0cb0 w0xlt: reACK https://github.com/bitcoin/bitcoin/commit/53e7ed075c49f853cc845afc7b2f058cabad0cb0 Tree-SHA512: c0c003694ca2e17064922d08e8464278d314e970efb7df874b4fe04ec5d124c7206409ca701c65c099d17779ab2136ae63f1da2a9dba39b45f6d62cf93b5c60a
2022-09-01Merge bitcoin/bitcoin#25931: rpc: sort listdescriptors resultAndrew Chow
50996241f2b0eefeaab4fcd11b9730fa2dc107ae rpc: sort listdescriptors result (Sjors Provoost) Pull request description: This puts receive and change descriptors directly below each other. The change would be simpler if `UniValue` arrays were sortable. ACKs for top commit: achow101: ACK 50996241f2b0eefeaab4fcd11b9730fa2dc107ae S3RK: reACK 50996241f2b0eefeaab4fcd11b9730fa2dc107ae furszy: utACK 50996241 w0xlt: reACK https://github.com/bitcoin/bitcoin/pull/25931/commits/50996241f2b0eefeaab4fcd11b9730fa2dc107ae Tree-SHA512: 71246a48ba6f97c3e7c76ee32ff9e958227a14ca5a6eec638215dbfee57264d4e918ea5837f4d030eddc9c797c93df1791ddd55b5a499522ce2a35bcf380670b
2022-08-31rpc: sort listdescriptors resultSjors Provoost
2022-08-31Merge bitcoin/bitcoin#25872: Fix issues when calling std::move(const&)fanquake
fa875349e22f2f0f9c2c98ee991372d08ff90318 Fix iwyu (MacroFake) faad673716cfbad1e715f1bdf8ac00938a055aea Fix issues when calling std::move(const&) (MacroFake) Pull request description: Passing a symbol to `std::move` that is marked `const` is a no-op, which can be fixed in two ways: * Remove the `const`, or * Remove the `std::move` ACKs for top commit: ryanofsky: Code review ACK fa875349e22f2f0f9c2c98ee991372d08ff90318. Looks good. Good for univalue to support c++11 move optimizations Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
2022-08-29Add migratewallet RPCAndrew Chow
2022-08-29Implement MigrateLegacyToDescriptorAndrew Chow
2022-08-29Implement MigrateToSQLiteAndrew Chow
2022-08-29wallet: Deduplicate Resend and ReacceptWalletTransactionsAndrew Chow
Both of these functions do almost the exact same thing. They can be deduplicated so that their behavior matches except for the filtering aspect. As this function will now always be called on wallet loading, nNextResend will also always be initialized, so wallet_resendwallettransactions.py is updated to account for that. This also resolves a bug where ResendWalletTransactions would fail to rebroadcast txs in insertion order thereby potentially rebroadcasting a child transaction before its parent and causing the child to not actually get rebroadcast. Also names the combined function to ResubmitWalletTransactions as the function just submits the transactions to the mempool rather than doing any sending by itself.
2022-08-26Merge bitcoin/bitcoin#25922: wallet: trigger MaybeResendWalletTxs() every minuteAndrew Chow
5ef8c2c9fc4ebce6cbfea6a55a89a0ab7ee98a1a test: fix typo for MaybeResendWalletTxs (stickies-v) fbba4a131647c991afc53b6a3dfb9721f5c430b2 wallet: trigger MaybeResendWalletTxs() every minute (stickies-v) Pull request description: ResendWalletTransactions() only executes every [12-36h (24h average)](https://github.com/bitcoin/bitcoin/blob/1420547ec30a24fc82ba3ae5ac18374e8e5af5e5/src/wallet/wallet.cpp#L1947). Triggering it every second is excessive, once per minute should be plenty. The goal of this PR is to reduce the amount of (unnecessary) schedule executions by ~60x without meaningfully altering transaction rebroadcast logic/assumptions which would require more significant review. ACKs for top commit: achow101: ACK 5ef8c2c9fc4ebce6cbfea6a55a89a0ab7ee98a1a 1440000bytes: ACK https://github.com/bitcoin/bitcoin/pull/25922/commits/5ef8c2c9fc4ebce6cbfea6a55a89a0ab7ee98a1a Tree-SHA512: 4a077e3579b289c11c347eaa0d3601ef2dbb9fee66ab918d56b4a0c2e08222560a0e6be295297a74831836e001a997ecc143adb0c132faaba96a669dac1cd9e6
2022-08-26Merge bitcoin/bitcoin#25896: wallet: Log when Wallet::SetMinVersion sets a ↵Andrew Chow
different minversion 835bd27e9a0dd627f266e3dc0a7422d8d0612eff Wallet::SetMinVersion - Log the new minversion (Ali Sherief) Pull request description: This change prints a single additional line in the debug.log when bitcoin-cli loads a wallet using `loadwallet` (*not* `createwallet`). When Bitcoin Core creates a wallet, it's `minversion` is set to `FEATURE_BASE`, which is 10500. However, once the wallet is unloaded using `unloadwallet` or through program termination, and subsequently loaded again, `loadwallet` updates the `minversion` in the wallet.dat file to `FEATURE_LATEST`, currently 169900. The current logging format prints the very old wallet version during `createwallet`, and then the actual version in calls to `loadwallet`. This has confused at least one person ([reference](https://bitcointalk.org/index.php?topic=5410650.0) - I was the one who asked there if there were plans to change that behavior, and was subsequently redirected here by achow), so it will be very helpful to users to explicitly specify in the logs what the walletdb is doing. ACKs for top commit: achow101: ACK 835bd27e9a0dd627f266e3dc0a7422d8d0612eff Tree-SHA512: 967c8c617e06a84915ddb147378ec3c8b0343e45f43145ec78df9cbc0201867f49c8e11cd068c403eb5ec06e07d38c3c0d3864dad8edc5efbb134a3fb30be41f
2022-08-26wallet: Refactor SetupDescSPKMs to take CExtKeyAndrew Chow
Refactors SetupDescSPKMs so that the DescSPKM loops are in their own function. This allows us to call it later during migration with a key that was already generated.
2022-08-26Implement LegacyScriptPubKeyMan::DeleteRecordsAndrew Chow
2022-08-25Implement LegacyScriptPubKeyMan::MigrateToDescriptorAndrew Chow
2022-08-25scriptpubkeyman: Implement GetScriptPubKeys in LegacyAndrew Chow
2022-08-25Apply label to all scriptPubKeys of imported combo()Andrew Chow
2022-08-25Wallet::SetMinVersion - Log the new minversionAli Sherief
2022-08-25wallet: trigger MaybeResendWalletTxs() every minutestickies-v
ResendWalletTransactions() only executes every 12-36h (24h average). Triggering it every second is excessive, once per minute should be plenty.
2022-08-24scripted-diff: rpc: fix rescan RPC name (s/rescanwallet/rescanblockchain/)Sebastian Falbesoner
There is no RPC call named `rescanwallet`, i.e. fix this by renaming to the actual RPC called `rescanblockchain`. -BEGIN VERIFY SCRIPT- sed -i s/rescanwallet/rescanblockchain/ $(git grep -l rescanwallet) -END VERIFY SCRIPT-
2022-08-22Merge bitcoin/bitcoin#25647: wallet: return change from SelectionResultAndrew Chow
4fef5344288e454460b80db0316294e1ec1ad8ad wallet: use GetChange() when computing waste (S3RK) 87e0ef903133492e76b7c7556209554d4a0c3d66 wallet: use GetChange() in tx building (S3RK) 15e97a6886902ebb378829993a972dc52558aa92 wallet: add SelectionResult::GetChange (S3RK) 72cad28da05cfce9e4950f2dc5a709da41d251f4 wallet: calculate and store min_viable_change (S3RK) e3210a722542a9cb5f7e4be72470dbe488c281fd wallet: account for preselected inputs in target (S3RK) f8e796348b644c011ad9a8312356d4426c16cc4b wallet: add SelectionResult::Merge (S3RK) 06f558e4e2164d1916f258c731efe4586728a23b wallet: accurate SelectionResult::m_target (S3RK) c8cf08ea743e430c2bf3fe46439594257b0937e5 wallet: ensure m_min_change_target always covers change fee (S3RK) Pull request description: Benefits: 1. more accurate waste calculation for knapsack. Waste calculation is now consistent with tx building code. Before we always assumed change for knapsack even when the solution is changeless4. 2. simpler tx building code. Only create change output when it's needed 3. makes it easier to correctly account for fees for CPFP inputs (should be done in a follow up) In the first three commits we fix the code to accurately track selection target in `SelectionResult::m_target` Then we introduce new variable `min_change` that represents the minimum viable change amount Then we introduce `SelectionResult::GetChange()` which incapsulates dropping change for fee logic and uses correct values of `SelectionResult::m_target` Then we use `SelectionResult::GetChange()` in both tx building and waste calculation code This PR is a refactoring and shouldn't change the behaviour. There is only one known small change (arguably a bug fix). Before we dropped change output if it's smaller than `cost_of_change` after paying change fees. This is incorrect as `cost_of_change` already includes `change_fee`. ACKs for top commit: achow101: ACK 4fef5344288e454460b80db0316294e1ec1ad8ad Xekyo: crACK 4fef5344288e454460b80db0316294e1ec1ad8ad furszy: Code review ACK 4fef5344 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25647/commits/4fef5344288e454460b80db0316294e1ec1ad8ad Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941
2022-08-22fixups for BIP125 doc cleanupglozow
Grammar and readability fixups. Clarifies "bip125-replaceable" helpstrings.
2022-08-22Merge bitcoin/bitcoin#25775: docs: remove non-signaling mentions of BIP125fanquake
1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e [doc] remove non-signaling mentions of BIP125 (glozow) 32024d40f03fbf47c64d814fa5f2c2a73ec14cb7 scripted-diff: remove mention of BIP125 from non-signaling var names (glozow) Pull request description: We have pretty thorough documentation of our RBF policy in doc/policy/mempool-replacements.md. It enumerates each rule with several sentences of rationale. Also, each rule pretty much has its own function (3 and 4 share one), with extensive comments. The doc states explicitly that our rules are similar but differ from BIP125, and contains a record of historical changes to RBF policy. We should not use "BIP125" as synonymous with our RBF policy because: - Our RBF policy is different from what is specified in BIP125, for example: - the BIP does not mention our rule about the replacement feerate being higher (our Rule 6) - the BIP uses minimum relay feerate for Rule 4, while we have used incremental relay feerate since #9380 - the "inherited signaling" question (CVE-2021-31876). Call it discrepancy, ambiguous wording, doc misinterpretation, or implementation details, I would recommend users refer to doc/policy/mempool-replacements.md - the signaling policy is configurable, see #25353 - Our RBF policy may change further - We have already marked BIP125 as only "partially implemented" in docs/bips.md since 1fd49eb498c75a1d14193bb736d195a3dc75ae12 - See comments from people who are not me recently: - https://github.com/bitcoin/bitcoin/pull/25038#discussion_r909507429 - https://github.com/bitcoin/bitcoin/pull/25575#issuecomment-1179519204 This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period). It may be useful to refer to the concept of "tx opts in to RBF if it has at least one nSequence less than (0xffffffff - 1)" as "BIP125 signaling" because: - It is succint. - It has already been widely marketed as BIP125 opt-in signaling. - Our API uses it when referring to signaling (e.g. getmempoolentry["bip125-replaceable"] and wallet error message "not BIP 125 replaceable"). Changing those is more invasive. - If/when we have other ways to signal in the future, we can disambiguate them this way. See #25038 which proposes another way of signaling, and where I pulled these commits from. Alternatives: - Changing our policy to match BIP125. This doesn't make sense as, for example, we would have to remove the requirement that a replacement tx has a higher feerate (Rule 6). - Changing BIP125 to match what we have. This doesn't make sense as it would be a significant change to a BIP years after it was finalized and already used as a spec to implement RBF in other places. - Document our policy as a new BIP and give it a number. This might make sense if we don't expect things to change a lot, and can be done as a next step. ACKs for top commit: darosior: ACK 1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e ariard: ACK 1dc03dda t-bast: ACK https://github.com/bitcoin/bitcoin/commit/1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e Tree-SHA512: a3adc2039ec5785892d230ec442e50f47f7062717392728152bbbe27ce1c564141f85253143f53cb44e1331cf47476d74f5d2f4b3cd873fc3433d7a0aa783e02
2022-08-22Merge bitcoin/bitcoin#23202: wallet: allow psbtbumpfee to work with txs with ↵fanquake
external inputs c3b099ace031758cafeec08c38bedbf717d6b7fe wallet, tests: Test bumpfee's max input weight calculation (Andrew Chow) 116a620ce7e6724906d63de80a8a757004f22477 Make DUMMY_CHECKER availble outside of script/sign.cpp (Andrew Chow) ff638323d1cde68b537bb20cf096cba4e88ac4eb test, bumpfee: Check that psbtbumpfee can bump txs with external inputs (Andrew Chow) 1bc8106d4cb75f7d4862d4651f30bd2df9cfeb34 bumpfee: be able to bump fee of a tx with external inputs (Andrew Chow) 31dd3dc9e5b27fa2bbb5170ad98107a36fe55958 bumpfee: Clear scriptSigs and scriptWitnesses before calculated max size (Andrew Chow) a0c3afb898016c2e0a76dc48f68eaa5c3ae6282c bumpfee: extract weights of external inputs when bumping fee (Andrew Chow) 612f1e44fe7ead319ae87653607614dd1bc14d60 bumpfee: Calculate fee by looking up UTXOs (Andrew Chow) Pull request description: This PR allows `psbtbumpfee` to return a PSBT for transactions that contain external inputs. This does not work for bumping in the GUI nor `bumpfee` because these need private keys available to sign and send the transaction. But `psbtbumpfee` returns a psbt, so it is fine to not be able to sign. In order to correctly estimate the size of the inputs for coin selection, the fee bumper will use the size of the inputs of the transaction being bumped. Because the sizes of signatures are not guaranteed, for external inputs, the fee bumper will verify the scripts with a special SignatureChecker which will compute the weight of all of the signatures in that input, and compute their weights if those signatures were maximally sized. This allows the fee bumper to obtain a max size estimate for each external input. Builds on #23201 as it relies on the ability to pass weights in to coin selection. Closes #23189 ACKs for top commit: ishaanam: reACK c3b099ace031758cafeec08c38bedbf717d6b7fe t-bast: Re-ran my tests agains https://github.com/bitcoin/bitcoin/pull/23202/commits/c3b099ace031758cafeec08c38bedbf717d6b7fe, ACK Tree-SHA512: 40016ec52d351430977579cfa2694c7e6764f42c9ce09d3a6f1753b767f86053f296d9de988248df033be6d725d67badbf2a5ef82c8ace23c61487729b7691e5
2022-08-20Fix issues when calling std::move(const&)MacroFake
2022-08-19wallet, tests: Test bumpfee's max input weight calculationAndrew Chow
2022-08-19Merge bitcoin/bitcoin#25784: Wallet: Document expectations for ↵Andrew Chow
AddWalletFlags (now InitWalletFlags) correctly 0cb6d2aec63aec76a517b8da621a3c53ab432632 Bugfix: Wallet: Document expectations for AddWalletFlags (now InitWalletFlags) correctly (Luke Dashjr) Pull request description: Includes some slight refactoring (return type changed, current status checked) ACKs for top commit: achow101: ACK 0cb6d2aec63aec76a517b8da621a3c53ab432632 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25784/commits/0cb6d2aec63aec76a517b8da621a3c53ab432632 ryanofsky: Code review ACK 0cb6d2aec63aec76a517b8da621a3c53ab432632. This is a clarifying change, and should prevent the InitWalletFlags method being called incorrectly. I left a comment suggestion, but feel free to ignore it. Tree-SHA512: fa18e9471b5e89d35cbc01526e6d4dbe4eee8faa9646847248909af1751b33014a6f9a42fe70a1331c0d73adea79008b8fc3ae2b51a641eba3e36d5c631327f6
2022-08-19Merge bitcoin/bitcoin#25869: wallet: remove UNKNOWN type from OUTPUT_TYPES arrayAndrew Chow
5b4fdbbff527aef8288edb3bf21b478de1061221 wallet: remove UNKNOWN type from OUTPUT_TYPES array (furszy) Pull request description: Fixing https://github.com/bitcoin/bitcoin/pull/25734#discussion_r949502998 -> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50329 The `OUTPUT_TYPES` array contain the known active output types only. And it's solely used to create/walk-through the active spkms. So, no need to add the `UNKNOWN` type here. ACKs for top commit: achow101: ACK 5b4fdbbff527aef8288edb3bf21b478de1061221 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25869/commits/5b4fdbbff527aef8288edb3bf21b478de1061221 LarryRuane: ACK 5b4fdbbff527aef8288edb3bf21b478de1061221 Tree-SHA512: dee2dc362a1b0c777555e5ee4d355a3351340591d0096f74e8c3a25f374cb2d9aef26145977ff4dd0f8cc940da9464eb5541eb2895bc19f8cbd6bb6d292ab9a9
2022-08-19bumpfee: be able to bump fee of a tx with external inputsAndrew Chow
In some cases, notably psbtbumpfee, it is okay, and potentially desired, to be able to bump the fee of a transaction which contains external inputs.
2022-08-19bumpfee: Clear scriptSigs and scriptWitnesses before calculated max sizeAndrew Chow
The max size calculation expects some inputs to have empty scriptSigs and witnesses, so we need to clear these before doing that calculation.
2022-08-19bumpfee: extract weights of external inputs when bumping feeAndrew Chow
When bumping the fee of a transaction containing external inputs, determine the weights of those inputs. Because signatures can have a variable size, the script is executed with a special SignatureChecker which will compute the total weight of the signatures in the transaction and the weight if they were all maximum size signatures. This allows us to compute the maximum weight of the input for use during coin selection.
2022-08-19bumpfee: Calculate fee by looking up UTXOsAndrew Chow
Instead of calculating the fee by using what is stored in the wallet, calculate it by looking up the UTXOs.
2022-08-19Merge bitcoin/bitcoin#25707: refactor: Make const references to avoid ↵MacroFake
unnecessarily copying objects and enable two clang-tidy checks ae7ae36d311a869b3bda41d29dc0e47fade77d28 tidy: Enable two clang-tidy checks (Aurèle Oulès) 081b0e53e3adca7ea57d23e5fcd9db4b86415a72 refactor: Make const refs vars where applicable (Aurèle Oulès) Pull request description: I added const references to some variables to avoid unnecessarily copying objects. Also added two clang-tidy checks : [performance-for-range-copy](https://releases.llvm.org/11.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-for-range-copy.html) and [performance-unnecessary-copy-initialization](https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.html). ACKs for top commit: vasild: ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28 MarcoFalke: review ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28 Tree-SHA512: f6ac6b0cd0eee1e0c34d2f186484bc0f7ec6071451cccb33fa88a67d93d92b304e2fac378b88f087e94657745bca4e966dbc443759587400eb01b1f3061fde8c
generated by cgit v1.2.3 (git 2.26.2) at 2025-03-17 04:44:37 +0000