aboutsummaryrefslogtreecommitdiff
path: root/src/wallet/spend.cpp
AgeCommit message (Collapse)Author
2024-07-17Merge bitcoin/bitcoin#29523: Wallet: Add `max_tx_weight` to transaction ↵Ava Chow
funding options (take 2) 734076c6de1781f957c8bc3bf7ed6951920cfcf6 [wallet, rpc]: add `max_tx_weight` to tx funding options (ismaelsadeeq) b6fc5043c16c2467a2a6768a6ca9b18035fc400f [wallet]: update the data type of `change_output_size`, `change_spend_size` and `tx_noinputs_size` to `int` (ismaelsadeeq) baab0d2d43049a71dc90176bc4d72062f7b2ce19 [doc]: update reason for deducting change output weight (ismaelsadeeq) 7f61d31a5cec8fc61328bee43f90d3f1dcb0a035 [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 734076c6de1781f957c8bc3bf7ed6951920cfcf6 furszy: utACK 734076c6de1781f957c8bc3bf7ed6951920cfcf6 rkrux: reACK [734076c](https://github.com/bitcoin/bitcoin/pull/29523/commits/734076c6de1781f957c8bc3bf7ed6951920cfcf6) Tree-SHA512: 013501aa443d239ee2ac01bccfc5296490c27b4edebe5cfca6b96c842375e895e5cfeb5424e82e359be581460f8be92095855763a62779a18ccd5bdfdd7ddce7
2024-07-06random: drop ad-hoc Shuffle in favor of std::shufflePieter Wuille
Benchmarks show it is no longer faster with modern standard C++ libraries, and the debug-mode failure due to self-move has been fixed as well.
2024-06-27Merge bitcoin/bitcoin#30050: refactor, wallet: get serialized size of ↵Ava Chow
`CRecipient`s directly a9c7300135f0188daa5bce5491e2daf2dd8da8ae move-only: refactor CreateTransactionInternal (josibake) adc6ab25bba42ce9e7ed6bd7599aabb6dead6987 wallet: use CRecipient instead of CTxOut (josibake) Pull request description: Broken out from #28201 --- In order to estimate fees properly, we need to know what the final serialized transaction size will be. This PR refactors `CreateTransactionInternal` to: * Get the serialized size directly from the `CRecipient`: this sets us up in a future PR to calculate the serialized size of silent payment `CTxDestinations` (see https://github.com/bitcoin/bitcoin/pull/28201/commits/797e21c8c1bf393e668eeef8bb7ee9cae1e5e41e) * Use the new `GetSerializeSizeForRecipient` to move the serialize size calculation to *before* coin selection and the output creation to *after* coin selection: this also sets us up for silent payments sending in a future PR in that silent payments outputs cannot be created until after the inputs to the transaction have been selected Aside from the silent payments use case, I think this structure logically makes more sense. As a reminder, move-only commits are best reviewed with something like `git diff -w --color-moved=dimmed-zebra` ACKs for top commit: S3RK: reACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae achow101: ACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae rkrux: tACK [a9c7300](https://github.com/bitcoin/bitcoin/pull/30050/commits/a9c7300135f0188daa5bce5491e2daf2dd8da8ae) Tree-SHA512: 412e1764b98f7428c8530c3a68f55e32063d6b66ab2ff613e1c7e12d49b049807cb60055cfe7f7e8ffe7ac7f0f9931427cbfd3efe7d4f97a5a0f6d1bf1aaac58
2024-06-27[wallet, rpc]: add `max_tx_weight` to tx funding optionsismaelsadeeq
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[wallet]: update the data type of `change_output_size`, `change_spend_size` ↵ismaelsadeeq
and `tx_noinputs_size` to `int` - This change ensures consistency in transaction size and weight calculation within the wallet and prevents conversion overflow when calculating `max_selection_weight`.
2024-06-27[doc]: update reason for deducting change output weightismaelsadeeq
`CoinGrinder` will also produce change output, listing all the Coin selection algorithms that produces change output is not maintainable, just infer that remaining algorithms all might produce change.
2024-06-27[refactor]: update coin selection algorithms input parameter `max_weight` nameismaelsadeeq
- This commit renames the coin selection algorithms input parameter `max_weight` to `max_selection_weight` for clarity. The parameter represent the maximum weight of the UTXOs the coin selection algorithm should select, not the transaction maximum weight. - The commit updates the parameter docstring to provide correct description. - Also updates coin selection unit and fuzzing test variables to match the new name.
2024-06-26Merge bitcoin/bitcoin#30309: wallet: notify when preset + automatic inputs ↵Ava Chow
exceed max weight 72b226882fe2348a9a66aee1d8d21b4e2d275e68 wallet: notify when preset + automatic inputs exceed max weight (furszy) Pull request description: Small change. Found it while finishing my review on #29523. This does not interfere with it. Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight. This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`. ACKs for top commit: achow101: ACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68 tdb3: re ACK for 72b226882fe2348a9a66aee1d8d21b4e2d275e68 rkrux: tACK [72b2268](https://github.com/bitcoin/bitcoin/pull/30309/commits/72b226882fe2348a9a66aee1d8d21b4e2d275e68) ismaelsadeeq: utACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68 Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
2024-06-21wallet: notify when preset + automatic inputs exceed max weightfurszy
This also avoids signing all inputs prior to erroring out.
2024-06-20refactor: remove extraneous lock annotations from function definitionsCory Fields
These annotations belong in the declarations rather than the definitions. While harmless now, future versions of clang may warn about these.
2024-06-17move-only: refactor CreateTransactionInternaljosibake
Move the output serialization size and dust calculation into the loop where the outputs are iterated over to calculate the total sum. Move the code for adding the the txoutputs to the transaction to after coin selection. While this code structure generally follows a more logical flow, the primary motivation for moving the code for adding outputs to the transaction sets us up nicely for silent payments (in a future PR): we need to know the input set before generating the final output scriptPubKeys.
2024-06-17wallet: use CRecipient instead of CTxOutjosibake
Now that a CRecipient holds a CTxDestination, we can get the serialized size and determine if the output is dust using the CRecipient directly. This does not change any current behavior, but provides a nice generalization that can be used to apply special logic to a CTxDestination serialization and dust calculations in the future. Specifically, in a later PR when support for `V0SilentPayment` destinations is added, we need to use `WitnessV1Taproot` as the scriptPubKey for serialized size calcuations whenever the `CRecipient` destination is a `V0SilentPayment` destination.
2024-06-12Merge bitcoin/bitcoin#29015: kernel: Streamline util libraryAva Chow
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky) 4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky) 4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky) 680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky) 02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky) 0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky) 9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky) 6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky) 23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan) 6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky) cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky) 5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky) ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 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 c7376babd19d0c858fef93ebd58338abd530c1f4 TheCharlatan: Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4 hebasto: re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4. Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
2024-06-07refactor: Rename CTransaction::nVersion to versionAva Chow
In order to ensure that the change of nVersion to a uint32_t in the previous commit has no effect, rename nVersion to version in this commit so that reviewers can easily spot if a spot was missed or if there is a check somewhere whose semantics have changed.
2024-06-04Merge bitcoin/bitcoin#28366: Fix waste calculation in SelectionResultAva Chow
bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624 Use `exact_target` shorthand in coinselector_tests (Murch) 7aa7e30441fe77bf8e8092916e36b004bbbfe2a7 Fold GetSelectionWaste() into ComputeAndSetWaste() (Murch) Pull request description: PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is problematic, because a `change_cost` of 0 is permitted with custom settings for feerate and discard_feerate (i.e. when they’re both 0). ACKs for top commit: achow101: ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624 furszy: Code ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624 ismaelsadeeq: Code Review ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624 Tree-SHA512: 83a2688d45d719dc61a64b5180fe136107faccf401a59df65245c05d701748a03e85ed56fde8c9b7ef39a3ab54374dd3718c559bda5b3f55dafedfd7fed25161
2024-05-24Fold GetSelectionWaste() into ComputeAndSetWaste()Murch
Both `GetSelectionWaste()` and `ComputeAndSetWaste()` now are part of `SelectionResult`. Instead of `ComputeAndSetWaste()` being a wrapper for `GetSelectionWaste()`, we combine them to a new function `RecalculateWaste()`. As I was combining the logic of the two functions, I noticed that `GetSelectionWaste()` was making the odd assumption that the `change_cost` being set to zero means that no change is created. However, if we build transactions at a feerate of zero with the `discard_feerate` also set to zero, we'd organically have a `change_cost` of zero, even when we create change on a transaction. This commit cleans up this duplicate meaning of `change_cost` and relies on `GetChange()` to figure out whether there is change on basis of the `min_viable_change` and whatever is left after deducting fees. Since this broke a bunch of tests that relied on the double-meaning of `change_cost` a bunch of tests had to be fixed.
2024-05-16util: add TransactionError includes and namespace declarationsRyan Ofsky
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-16util: move fees.h and error.h to common/messages.hRyan Ofsky
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-04-25refactor: Avoid copying util::Result valuesRyan Ofsky
Copying util::Result values is less efficient than moving them because they allocate memory and contain strings. Also this is needed to avoid compile errors in https://github.com/bitcoin/bitcoin/pull/25722 which adds a std::unique_ptr member to util::Result which implicity disables copying.
2024-02-09Merge bitcoin/bitcoin#27877: wallet: Add CoinGrinder coin selection algorithmAva Chow
13161ecf032b7a850686e5942c12222c8f3d0d52 opt: Skip over barren combinations of tiny UTXOs (Murch) b7672c7cdd87acb105639f475744094b53cc9891 opt: Skip checking max_weight separately (Murch) 1edd2baa37a69ff69c2eaeceaad1028f1968cbab opt: Cut if last addition was minimal weight (Murch) 5248e2a60d243b3d5c77ecd9e4c335daca399a48 opt: Skip heavier UTXOs with same effective value (Murch) 9124c73742287b06dfe6e8a94be56ace25f07b2c opt: Tiebreak UTXOs by weight for CoinGrinder (Murch) 451be19dc10381122f705bbb2127b083f1d598c6 opt: Skip evaluation of equivalent input sets (Murch) 407b1e3432b77511900b77be84d5d7450352f462 opt: Track remaining effective_value in lookahead (Murch) 5f84f3cc043c5fb15072f5072fee752eaa01a2ec opt: Skip branches with worse weight (Murch) d68bc74fb2e3ae4ae775ab544fe5b4ab46025abb fuzz: Test optimality of CoinGrinder (Murch) 67df6c629a2e9878b06c1903e90b67087eaa3688 fuzz: Add CoinGrinder fuzz target (Murch) 1502231229dbc32c94e80a2fc2959275c5d246ef coinselection: Track whether CG completed (Murch) 7488acc64685ae16e20b78e7ad018137f27fe404 test: Add coin_grinder_tests (Murch) 6cc9a46cd0f4ed80d4523bbef1508142e0c80d27 coinselection: Add CoinGrinder algorithm (Murch) 89d09566431f57034d9a7df32547ceb13d79c62c opt: Tie-break UTXO sort by waste for BnB (Murch) aaee65823c6e620bef5cc96d8026567e64d822fe doc: Document max_weight on BnB (Murch) Pull request description: ***Please refer to the [topic on Delving Bitcoin](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) discussing Gutter Guard/Coingrinder simulation results.*** Adds a coin selection algorithm that minimizes the weight of the input set while creating change. Motivations --- - At high feerates, using unnecessary inputs can significantly increase the fees - Users are upset when fees are relatively large compared to the amount sent - Some users struggle to maintain a sufficient count of UTXOs in their wallet Approach --- So far, Bitcoin Core has used a balanced approach to coin selection, where it will generate multiple input set candidates using various coin selection algorithms and pick the least wasteful among their results, but not explicitly minimize the input set weight. Under some circumstances, we _do_ want to minimize the weight of the input set. Sometimes changeless solutions require many or heavy inputs, and there is not always a changeless solution for Branch and Bound to find in the first place. This can cause expensive transactions unnecessarily. Given a wallet with sufficient funds, `CoinGrinder` will pick the minimal-waste input set for a transaction with a change output. The current implementation only runs `CoinGrinder` at feerates over 3×long-term-feerate-estimate (by default 30 ṩ/vB), which may be a decent compromise between our goal to reduce costs for the users, but still permit transactions at lower feerates to naturally reduce the wallet’s UTXO pool to curb bloat. Trade-offs --- Simulations for my thesis on coin selection ([see Section 6.3.2.1 [PDF]](https://murch.one/erhardt2016coinselection.pdf)) suggest that minimizing the input set for all transactions tends to grind a wallet’s UTXO pool to dust (pun intended): an approach selecting inputs per coin-age-priority (in effect similar to “largest first selection”) on average produced a UTXO pool with 15× the UTXO count as Bitcoin Core’s Knapsack-based Coin Selection then (in 2016). Therefore, I do not recommend running `CoinGrinder` under all circumstances, but only at extreme feerates or when we have another good reason to minimize the input set for other reasons. In the long-term, we should introduce additional metrics to score different input set candidates, e.g. on basis of their privacy and wallet health impact, to pick from all our coin selection results, but until then, we may want to limit use of `CoinGrinder` in other ways. ACKs for top commit: achow101: ACK 13161ecf032b7a850686e5942c12222c8f3d0d52 sr-gi: ACK [13161ec](https://github.com/bitcoin/bitcoin/pull/27877/commits/13161ecf032b7a850686e5942c12222c8f3d0d52) sipa: ACK 13161ecf032b7a850686e5942c12222c8f3d0d52 Tree-SHA512: 895b08b2ebfd0b71127949b7dba27146a6d10700bf8590402b14f261e7b937f4e2e1b24ca46de440c35f19349043ed2eba4159dc2aa3edae57721384186dae40
2024-02-09coinselection: Add CoinGrinder algorithmMurch
CoinGrinder is a DFS-based coin selection algorithm that deterministically finds the input set with the lowest weight creating a change output.
2024-01-23Merge bitcoin/bitcoin#28560: wallet, rpc: `FundTransaction` refactorAva Chow
18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d refactor: pass CRecipient to FundTransaction (josibake) 5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 refactor: simplify `CreateRecipients` (josibake) 47353a608dc6e20e5fd2ca53850d6f9aa3240d4a refactor: remove out param from `ParseRecipients` (josibake) f7384b921c3460c7a3cc7827a68b2c613bd98f8e refactor: move parsing to new function (josibake) 6f569ac903e5ddaac275996a5d0c31b2220b7b81 refactor: move normalization to new function (josibake) 435fe5cd96599c518e26efe444c9d94d1277996b test: add tests for fundrawtx and sendmany rpcs (josibake) Pull request description: ## Motivation The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`. As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO. I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments. ACKs for top commit: S3RK: reACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d josibake: > According to my `range-diff` nothing changed. reACK [18ad1b9](https://github.com/bitcoin/bitcoin/commit/18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d) achow101: ACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
2024-01-20Move TRACEx parameters to seperate linesRichard Myers
2024-01-20wallet: fix coin selection tracing to return -1 when no change posRichard Myers
2024-01-19refactor: pass CRecipient to FundTransactionjosibake
Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction` take a `CRecipient` vector directly. This allows us to remove SFFO logic from the wrapper RPC `FundTransaction` since the `CRecipient` objects have already been created with the correct SFFO values. This also allows us to remove SFFO from both `FundTransaction` function signatures. This sets us up in a future PR to be able to use these RPCs with BIP352 static payment codes.
2023-12-17wallet, mempool: propagete `checkChainLimits` error message to walletismaelsadeeq
Update CheckPackageLimits to use util::Result to pass the error message instead of out parameter. Also update test to reflect the error message from `CTxMempool` `CheckPackageLimits` output.
2023-12-12Merge bitcoin/bitcoin#28994: wallet: skip BnB when SFFO is enabledAndrew Chow
576bee88fd36e207b7288077626947a1fce0fc33 fuzz: disable BnB when SFFO is enabled (furszy) 05e5ff194c7722b4ebc2b9309fc0bf47b3cf1df7 test: add coverage for BnB-SFFO restriction (furszy) 0c5755761c3e544547899ad096121585dffa73df wallet: create tx, log resulting coin selection info (furszy) 5cea25ba795d6eb9ccc721d01560783ae576af34 wallet: skip BnB when SFFO is active (Murch) Pull request description: Solves #28918. Coming from https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1838626406 discussion. The intention is to decouple only the bugfix relevant commits from #28985, allowing them to be included in the 26.x release. This way, we can avoid disabling the coin selection fuzzing test for an entire release. Note: Have introduced few changes to the bug fix commit so that the unit tests pass without the additional burden introduced in #28985. ACKs for top commit: josibake: ACK https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33 murchandamus: ACK https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33 achow101: ACK 576bee88fd36e207b7288077626947a1fce0fc33 Tree-SHA512: f5d90eb3f3f524265afe4719495c9bf30f98b9af26cf039f7df5a7db977abae72caa7a3478cdd0ab10cd143bc1662e8fc5286b5bc10fc10f0dd582a45b45c31a
2023-12-08wallet: return CreatedTransactionResult from FundTransactionAndrew Chow
Instead of using the output parameters, return CreatedTransactionResult from FundTransaction in the same way that CreateTransaction does. Additionally, instead of modifying the original CMutableTransaction, the result from CreateTransactionInternal is used.
2023-12-08wallet: use optional for change position as an optional in CreateTransactionAndrew Chow
Instead of making -1 a magic number meaning no change or random change position, use an optional to have that meaning.
2023-12-08wallet: Explicitly preserve scriptSig and scriptWitness in CreateTransactionAndrew Chow
When creating a transaction with preset inputs, also preserve the scriptSig and scriptWitness for those preset inputs if they are provided (e.g. in fundrawtransaction).
2023-12-08wallet: Explicitly preserve transaction version in CreateTransactionAndrew Chow
We provide the preset nVersion to CCoinControl so that CreateTransactionInternal can be aware of it and set it in the produced transaction.
2023-12-08wallet: Explicitly preserve transaction locktime in CreateTransactionAndrew Chow
We provide the preset nLockTime to CCoinControl so that CreateTransactionInternal can be aware of it and set it in the produced transaction.
2023-12-08wallet: Set preset input sequence through coin controlAndrew Chow
2023-12-08wallet: Replace SelectExternal with SetTxOutAndrew Chow
Instead of having a separate CCoinControl::SelectExternal function, we can use the normal CCoinControl::Select function and explicitly use PreselectedInput::SetTxOut in the caller. The semantics of what an external input is remains.
2023-12-08coincontrol: Replace HasInputWeight with returning optional from GetAndrew Chow
2023-12-07wallet: create tx, log resulting coin selection infofurszy
Useful for understanding what is going on internally when the software is running. Debug issues, and provide more accurate feedback to users.
2023-12-07wallet: skip BnB when SFFO is activeMurch
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
2023-11-21Use Txid in COutpointdergoegge
2023-11-17Merge bitcoin/bitcoin#28878: Remove version field from GetSerializeSizefanquake
83986f464c59a6517f790a960a72574e167f3f72 Include version.h in fewer places (Anthony Towns) c7b61fd61b199cbefda660c9d394bb4035a49528 Convert some CDataStream to DataStream (Anthony Towns) 1410d300df7e57a895f2697d9849a2201021c973 serialize: Drop useless version param from GetSerializeSize() (Anthony Towns) bf574a75016123309b894da895ab1c7a81731933 serialize: drop GetSerializeSizeMany (Anthony Towns) efa9eb6d7c8012fe4ed85699d81c8fe5dd18da1e serialize: Drop nVersion from [C]SizeComputer (Anthony Towns) Pull request description: Drops the version field from `GetSerializeSize()`, simplifying the code in various places. Also drop `GetSerializeSizeMany()` (as just removing the version parameter could result in silent bugs) and remove unnecessary instances of `#include <version.h>`. ACKs for top commit: maflcko: ACK 83986f464c59a6517f790a960a72574e167f3f72 📒 theuni: ACK 83986f464c59a6517f790a960a72574e167f3f72. Tree-SHA512: 36617b6dfbb1b4b0afbf673e905525fc6d623d3f568d3f86e3b9d4f69820db97d099e83a88007bfff881f731ddca6755ebf1549e8d8a7762437dfadbf434c62e
2023-11-16Merge bitcoin/bitcoin#28605: Fix typosfanquake
43de4d3630274e1287179c86896ed4c2d8b9eff4 doc: fix typos (Sjors Provoost) Pull request description: This PR fixes typos found by lint-spelling.py using codespell 2.2.6. Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now. ACKs for top commit: pablomartin4btc: re ACK 43de4d3630274e1287179c86896ed4c2d8b9eff4 Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
2023-11-16serialize: Drop useless version param from GetSerializeSize()Anthony Towns
2023-11-07doc: fix typosSjors Provoost
As found by lint-spelling.py using codespell 2.2.6.
2023-10-20interfaces: Rename CalculateBumpFees methods to be compatible with capn'protoRyan Ofsky
2023-10-12tidy: modernize-use-emplaceMarcoFalke
2023-09-19Merge bitcoin/bitcoin#28246: wallet: Use CTxDestination in CRecipient ↵fanquake
instead of just scriptPubKey ad0c469d98c51931b98b7fd937c6ac3eeaed024e wallet: Use CTxDestination in CRecipient rather than scriptPubKey (Andrew Chow) 07d3bdf4ebc06825ea24ab6f7c87aef6a22238c6 Add PubKeyDestination for P2PK scripts (Andrew Chow) 1a98a51c666e9ae77364115775ec2e0ba984e8e0 Allow CNoDestination to represent a raw script (Andrew Chow) 8dd067088d41f021b357d7db5fa5f0a9f61edddc Make WitnessUnknown members private (Andrew Chow) Pull request description: For silent payments, we want to provide a `SilentPaymentsDestination` to be used as the recipient, which requires `CRecipient` to use something other than just the `scriptPubKey` as we cannot know the output script for a silent payment prior to transaction creation. `CTxDestination` seems like the obvious place to add a `SilentPaymentsDestination` as it is our internal representation of an address. In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or the user hand crafted a raw transaction that they have given to `fundrawtransaction`), `CNoDestination` is changed to contain raw scripts. Additionally, P2PK scripts are now interpreted as a new `PubKeyDestination` rather than `PKHash`. This results in some things that would have given an address for P2PK scripts to no longer do so. This is arguably more correct. `ExtractDestination`'s behavior is slightly changed for the above. It now returns `true` for those destinations that have addresses, so P2PK scripts now result in `false`. Even though it returns false for `CNoDestination`, the script will now be included in that `CNoDestination`. Builds on #28244 ACKs for top commit: josibake: ACK https://github.com/bitcoin/bitcoin/pull/28246/commits/ad0c469d98c51931b98b7fd937c6ac3eeaed024e Tree-SHA512: ef3f8f3c7284779d9806c77c85b21caf910a79a1f7e7f1b51abcc0d7e074f14e00abf30f625a13075e41d94dad6202c10ddff462c0ee74c2ca4aab585b145a52
2023-09-14Merge bitcoin/bitcoin#26152: Bump unconfirmed ancestor transactions to ↵Andrew Chow
target feerate f18f9ef4d31c70e2d71ab90a24511692821418c3 Amend bumpfee for inputs with overlapping ancestry (Murch) 2e35e944dab09eff30952233f8dfc0b12c4553d5 Bump unconfirmed parent txs to target feerate (Murch) 3e3e05241128f68cf12f73ee06ff997395643885 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow) c57889da6650715f3e1153b6104bbdae15fcac90 [node] interface to get bump fees (glozow) c24851be945b2a633ee44ed3c8a501eee5580b62 Make MiniMinerMempoolEntry fields private (Murch) ac6030e4d8f7d578cd4a8593f41189efca548064 Remove unused imports (Murch) d2f90c31ef3b8dee5a3e0804ecc62fa1cfec7cd5 Fix calculation of ancestor set feerates in test (Murch) a1f7d986e0211e54e21a1d4a570e5f15294dca72 Match tx names to index in miniminer overlap test (Murch) Pull request description: Includes some commits to address follow-ups from #27021: https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1554675156 Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate. While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively. Fixes #9645 Fixes #9864 Fixes #15553 ACKs for top commit: S3RK: ACK f18f9ef4d31c70e2d71ab90a24511692821418c3 ismaelsadeeq: ACK f18f9ef4d31c70e2d71ab90a24511692821418c3 achow101: ACK f18f9ef4d31c70e2d71ab90a24511692821418c3 brunoerg: crACK f18f9ef4d31c70e2d71ab90a24511692821418c3 t-bast: ACK https://github.com/bitcoin/bitcoin/pull/26152/commits/f18f9ef4d31c70e2d71ab90a24511692821418c3, I reviewed the latest changes and run e2e tests against eclair, everything looks good :+1: Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6
2023-09-13Amend bumpfee for inputs with overlapping ancestryMurch
At the end of coin selection reduce the fees by the difference between the individual bump fee estimates and the collective bump fee estimate.
2023-09-13Bump unconfirmed parent txs to target feerateMurch
When a transaction uses an unconfirmed input, preceding this commit it would not consider the feerate of the parent transaction. Given a parent transaction with a lower ancestor feerate, this resulted in the new transaction's ancestor feerate undershooting the target feerate. This commit changes how we calculate the effective value of unconfirmed UTXOs. The effective value of unconfirmed UTXOs is decreased by the fee necessary to bump its ancestry to the target feerate. This also impacts the calculation of the waste metric: since the estimate for the current fee is increased by the bump fees, unconfirmed UTXOs current fees appear less favorable compared to their unchanged long term fees. This has one caveat: if multiple UTXOs have overlapping ancestries, each of their individual estimates will account for bumping all ancestors.
2023-09-12wallet: Use CTxDestination in CRecipient rather than scriptPubKeyAndrew Chow
2023-09-12Add PubKeyDestination for P2PK scriptsAndrew Chow
P2PK scripts are not PKHash destinations, they should have their own type. This also results in no longer showing a p2pkh address for p2pk outputs. However for backwards compatibility, ListCoinst will still do this conversion.