50996241f2 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 50996241f2
S3RK:
reACK 50996241f2
furszy:
utACK 50996241
w0xlt:
reACK 50996241f2
Tree-SHA512: 71246a48ba6f97c3e7c76ee32ff9e958227a14ca5a6eec638215dbfee57264d4e918ea5837f4d030eddc9c797c93df1791ddd55b5a499522ce2a35bcf380670b
fa875349e2 Fix iwyu (MacroFake)
faad673716 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 fa875349e2. Looks good. Good for univalue to support c++11 move optimizations
Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
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.
5ef8c2c9fc test: fix typo for MaybeResendWalletTxs (stickies-v)
fbba4a1316 wallet: trigger MaybeResendWalletTxs() every minute (stickies-v)
Pull request description:
ResendWalletTransactions() only executes every [12-36h (24h average)](1420547ec3/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 5ef8c2c9fc
1440000bytes:
ACK 5ef8c2c9fc
Tree-SHA512: 4a077e3579b289c11c347eaa0d3601ef2dbb9fee66ab918d56b4a0c2e08222560a0e6be295297a74831836e001a997ecc143adb0c132faaba96a669dac1cd9e6
835bd27e9a 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 835bd27e9a
Tree-SHA512: 967c8c617e06a84915ddb147378ec3c8b0343e45f43145ec78df9cbc0201867f49c8e11cd068c403eb5ec06e07d38c3c0d3864dad8edc5efbb134a3fb30be41f
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.
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-
4fef534428 wallet: use GetChange() when computing waste (S3RK)
87e0ef9031 wallet: use GetChange() in tx building (S3RK)
15e97a6886 wallet: add SelectionResult::GetChange (S3RK)
72cad28da0 wallet: calculate and store min_viable_change (S3RK)
e3210a7225 wallet: account for preselected inputs in target (S3RK)
f8e796348b wallet: add SelectionResult::Merge (S3RK)
06f558e4e2 wallet: accurate SelectionResult::m_target (S3RK)
c8cf08ea74 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 4fef534428
Xekyo:
crACK 4fef534428
furszy:
Code review ACK 4fef5344
w0xlt:
ACK 4fef534428
Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941
1dc03dda05 [doc] remove non-signaling mentions of BIP125 (glozow)
32024d40f0 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 1fd49eb498
- 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 1dc03dda05
ariard:
ACK 1dc03dda
t-bast:
ACK 1dc03dda05
Tree-SHA512: a3adc2039ec5785892d230ec442e50f47f7062717392728152bbbe27ce1c564141f85253143f53cb44e1331cf47476d74f5d2f4b3cd873fc3433d7a0aa783e02
c3b099ace0 wallet, tests: Test bumpfee's max input weight calculation (Andrew Chow)
116a620ce7 Make DUMMY_CHECKER availble outside of script/sign.cpp (Andrew Chow)
ff638323d1 test, bumpfee: Check that psbtbumpfee can bump txs with external inputs (Andrew Chow)
1bc8106d4c bumpfee: be able to bump fee of a tx with external inputs (Andrew Chow)
31dd3dc9e5 bumpfee: Clear scriptSigs and scriptWitnesses before calculated max size (Andrew Chow)
a0c3afb898 bumpfee: extract weights of external inputs when bumping fee (Andrew Chow)
612f1e44fe 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 c3b099ace0
t-bast:
Re-ran my tests agains c3b099ace0, ACK
Tree-SHA512: 40016ec52d351430977579cfa2694c7e6764f42c9ce09d3a6f1753b767f86053f296d9de988248df033be6d725d67badbf2a5ef82c8ace23c61487729b7691e5
0cb6d2aec6 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 0cb6d2aec6
w0xlt:
ACK 0cb6d2aec6
ryanofsky:
Code review ACK 0cb6d2aec6. 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
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.
ef8e2a5b09 tests: Test that external inputs of txs in wallet is handled correctly (Andrew Chow)
eb879634db wallet: Try estimating input size with external data if wallet fails (Andrew Chow)
a537d7aaa0 wallet: SelectExternal actually external inputs (Andrew Chow)
f2d00bfe1a wallet: Add CWallet::IsMine(COutPoint) (Andrew Chow)
Pull request description:
if a transaction is being funded that has an external input, and that input's parent is also in the wallet, we will fail to detect that and fail to fund the transaction. In order to correctly detect such inputs, we need to be doing `IsMine` on all specified inputs in order to use `Select` and `SelectExternal` correctly. Additionally `SelectCoins` needs to call `CalculateMaximumSignedInputSize` with the correct parameters which depends on whether the wallet is able to solve for the input. Because there are some situations where the wallet could find an external input to belong to it (e.g. watching an address - unable to solve, but will be ISMINE_WATCHONLY), instead of switching which `CalculateMaximumSignedInputSize` to use, we should call the one that uses the wallet, and if that fails, try again with the one that uses external solving data.
Also adds a test for this case.
ACKs for top commit:
instagibbs:
ACK ef8e2a5b09
furszy:
ACK ef8e2a5b
ishaanam:
reACK ef8e2a5b09
Tree-SHA512: a43c4aefeed4605f33a36ce87ebb916e2c153fea6d415b02c9a89275e84a7e3bf12840b33c296d2d2bde46350390da48d9262f9567338e3f21d5936aae4caa1e
Instead of choosing whether to use the wallet or external data when
estimating the size of an input, first use the wallet, then try external
data if that failed.
If an external input's utxo was created by a transaction that the wallet
knows about, then it would not be selected using SelectExternal. This
results in either funding failure or incorrect weight calculation.
fa3f15f2dd refactor: Avoid copies in FlatSigningProvider Merge (MacroFake)
Pull request description:
`Merge` will create several copies unconditionally:
* To initialize the args `a`, and `b`
* `ret`, which is the merge of the two args
So change the code to let the caller decide how many copies they need/want:
* `a`, and `b` must be explicitly moved or copied by the caller
* `ret` is no longer needed, as `a` can be used for it in place "for free"
ACKs for top commit:
achow101:
ACK fa3f15f2dd
furszy:
looks good, ACK fa3f15f2
ryanofsky:
Code review ACK fa3f15f2dd. Confirmed that all the places `std::move` was added the argument actually did seem safe to move from. Compiler enforces that temporary copies are explicitly created in non-move cases.
Tree-SHA512: 7c027ccdea1549cd9f37403344ecbb76e008adf545f6ce52996bf95e89eb7dc89af6cb31435a9289d6f2eea1c416961b2fb96348bc8a211d550728f1d99ac49c
8cd21bb279 refactor: improve readability for AttemptSelection (josibake)
f47ff71761 test: only run test for descriptor wallets (josibake)
0760ce0b9e test: add missing BOOST_ASSERT (josibake)
db09aec937 wallet: switch to new shuffle, erase, push_back (josibake)
b6b50b0f2b scripted-diff: Uppercase function names (josibake)
3f27a2adce refactor: add new helper methods (josibake)
f5649db9d5 refactor: add UNKNOWN OutputType (josibake)
Pull request description:
This PR is to address follow-ups for #24584, specifically:
* Remove redundant, hard-to-read code by adding a new `OutputType` and adding shuffle, erase, and push_back methods for `CoinsResult`
* Add missing `BOOST_ASSERT` to unit test
* Ensure functional test only runs if using descriptor wallets
* Improve readability of `AttemptSelection` by removing triple-nested if statement
Note for reviewers: commit `refactor: add new helper methods` should throw an "unused function warning"; the function is used in the next commit. Also, commit `wallet: switch to new shuffle, erase, push_back` will fail to compile, but this is fixed in the next commit with a scripted-diff. the commits are separate like this (code change then scripted-diff) to improve legibility.
ACKs for top commit:
achow101:
ACK 8cd21bb279
aureleoules:
ACK 8cd21bb279.
LarryRuane:
Concept, code review ACK 8cd21bb279
furszy:
utACK 8cd21bb2. Left a small, non-blocking, comment.
Tree-SHA512: a1bbc5962833e3df4f01a4895d8bd748cc4c608c3f296fd94e8afd8797b8d2e94e7bd44d598bd76fa5c9f5536864f396fcd097348fa0bb190a49a86b0917d60e
a6b0c1fcc0 doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot)
0fd2d14454 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot)
55f98d087e rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot)
b724476158 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot)
55a82eaf91 wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot)
Pull request description:
Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends).
It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit.
I'll add this to the output of `listunspent` too if this gets a few Concept ACKs.
ACKs for top commit:
instagibbs:
ACK a6b0c1fcc0
achow101:
re-ACK a6b0c1fcc0
Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37
It's useful for an external application tracking coins to not be limited
by our change detection. For instance, for a watchonly wallet with two
descriptors a transaction from one to the other would be considered a
change output and not be included in the result (if the address was not
generated by this wallet).
This is a refactor, putting the burden to think about thread safety to
the caller. Otherwise, there is a risk that the caller will assume
thread safety where none exists, as is evident in the previous two
commits.
When we have preselected inputs the coin selection search target is reduced
by the sum of (effective) values. This causes incorrect m_target value.
Create separate instance of SelectionResult for all the preselected inputs and
set the target equal to the sum of (effective) values. Target for preselected
SelectionResult is equal to the delta for the search target. To get the final
SelectionResult with accurate m_target we merge both SelectionResult instances.
SelectionResult::m_target should be equal to actual selection target.
Selection target is the sum of all recipient amounts plus non input fees.
So we need to remove change_fee from the m_target. It's safe because change
target is always greater than the change fee, so we can always cover fees
if change output is created.
292b1a3e9c GetExternalSigner(): fail if multiple signers are found (amadeuszpawlik)
Pull request description:
If there are multiple external signers, `GetExternalSigner()` will
just pick the first one in the list. If the user has two or more
hardware wallets connected at the same time, he might not notice this.
This PR adds a check and fails with suitable message, forcing the user to disconnect all but one external signer, so that there is no ambiguity as to which external signer was used.
ACKs for top commit:
Sjors:
tACK 292b1a3e9c
achow101:
ACK 292b1a3e9c
Tree-SHA512: e2a41d3eecc607d4f94e708614bed0f3545f7abba85f300c5a5f0d3d17d72c815259734accc5ca370953eacd290f27894ba2c18016f5e9584cd50fa1ec2fbb0b
Move TopUp() responsibility from the wallet class to each scriptpubkeyman.
So each spkm can decide to call it or not after perform the basic checks
for the new destination request.
Reason:
We were calling it twice in the following flows for descriptor wallets:
A) CWallet::GetNewDestination:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again.
B) CWallet::GetReservedDestination:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again).
acda7e8686 [coin selection] consolidate m_change_target and m_min_change_target (glozow)
Pull request description:
These values are both intended for the same thing. Their divergence seems to be the result of an incomplete rename.
ACKs for top commit:
achow101:
ACK acda7e8686
Xekyo:
ACK acda7e8686
furszy:
ACK acda7e86
aureleoules:
ACK acda7e8686.
Tree-SHA512: 4b86171af5d893f7172373bb404bad12c49588ad1e22eb0544c242173f4bc4dede2ff1270c93c9f02f503ab8d9f66b841a8319d0ecb5e896d0fe8727cf03dbf4
b16f93cadd script/sign: remove needless IsSolvable() utility (Antoine Poinsot)
c232ef20c0 outputtype: remove redundant check for uncompressed keys in AddAndGetDestinationForScript (Antoine Poinsot)
Pull request description:
Now that we have descriptors there is no need to try to sign for a scriptPubKey using dummy signatures, and using a mocked verification of this witness against the interpreter, just to make sure we know how to spend such a Script. Just try to infer a solvable descriptor: any scriptPubKey that we can sign for can be inferred as such.
This came up in #24149 but i think it's worth it on its own.
ACKs for top commit:
instagibbs:
ACK b16f93cadd
achow101:
re-ACK b16f93cadd
furszy:
ACK b16f93ca, only change is the `IsSolvable` helper function removal.
Tree-SHA512: 137068157ce90210b710b1bf9ac3c400e2ff5af1112f892094b69875ea473d6a899f52adb51e5030cb907dee517602059cd1661107808558efa5de842ba12b41
fb9faffae3 extended keys: fail to derive too large depth instead of wrapping around (Antoine Poinsot)
8dc6670ce1 descriptor: don't assert success of extended key derivation (Antoine Poinsot)
50cfc9e761 (pubk)key: mark Derive() as nodiscard (Antoine Poinsot)
0ca258a5ac descriptor: never ignore the return value when deriving an extended key (Antoine Poinsot)
d3599c22bd spkman: don't ignore the return value when deriving an extended key (Antoine Poinsot)
Pull request description:
We would previously silently wrap the derived child's depth back to `0`. Instead, explicitly fail when trying to derive an impossible depth, and handle the error in callers.
An extended fuzzing corpus of `descriptor_parse` triggered this behaviour, which was reported by MarcoFalke.
Fixes#25751.
ACKs for top commit:
achow101:
re-ACK fb9faffae3
instagibbs:
utACK fb9faffae3
Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343
it was pointed out by a few reviewers that the code block at the end
of attempt selection was difficult to follow and lacked comments.
refactor to get rid of triple nested if statement and improve
readibility.
switch to new methods, remove old code. this also
updates the Size, All, and Clear methods to now use
the coins map.
this commit is not strictly a refactor because previously
coin selection was never run over the UNKNOWN type until the last
step when being run over all. now that we are iterating over each,
it is run over UNKNOWN but this is expected to be empty most of the time.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
add Shuffle, Erase, and Add to CoinsResult struct
add a helper function for mapping TxoutType to OutputType
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
76b3c37fcb refactor: wallet: return util::Result from `GetReservedDestination` methods (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #25218, as suggested in comment https://github.com/bitcoin/bitcoin/pull/25218#discussion_r907710067. The interfaces of the methods `ReserveDestination::GetReservedDestination`, `{Legacy,Descriptor,}ScriptPubKeyMan::GetReservedDestination` are improved by returning `util::Result<CTxDestination>` instead of `bool` in order to get rid of the two `CTxDestination&` and `bilingual_str&` out-parameters.
ACKs for top commit:
furszy:
ACK 76b3c37f
Tree-SHA512: bf15560a88d645bcf8768024013d36012cd65caaa4a613e8a055dfd8f29cb4a219c19084606992bad177920cdca3a732ec168e9b9526f9295491f2cf79cc6815
b5a762a353 wallet: improve `{LoadActive,Deactivate}ScriptPubKeyMan` log (w0xlt)
Pull request description:
This PR includes the output type description in the log. It currently shows the enum position, which is only useful if the reader knows the code.
Master:
```
Setting spkMan to active: id = 9f..04, type = 3, internal = 0
Setting spkMan to active: id = 3d..21, type = 2, internal = 0
Setting spkMan to active: id = 69..d4, type = 0, internal = 1
Setting spkMan to active: id = 97..ea, type = 1, internal = 1
```
PR:
```
Setting spkMan to active: id = 6a..4f, type = bech32m, internal = false
Setting spkMan to active: id = 83..dc, type = legacy, internal = true
Setting spkMan to active: id = 7e..5d, type = p2sh-segwit, internal = true
Setting spkMan to active: id = bd..d2, type = bech32, internal = true
Setting spkMan to active: id = 13...7c, type = bech32m, internal = true
```
ACKs for top commit:
S3RK:
Code review ACK b5a762a353
achow101:
ACK b5a762a353
theStack:
Code-review ACK b5a762a353
Tree-SHA512: 5a79706d5452e523b0456fb8435545c6c8e550b6722c0d7966af79011275a97ed97cab297562e031d601aa855118082c5b770af118783b1faaaec0cba9f9ee6a
bc886fcb31 Change mapWallet to be a std::unordered_map (Andrew Chow)
272356024d Change getWalletTxs to return a set instead of a vector (Andrew Chow)
97532867cf Change mapTxSpends to be a std::unordered_multimap (Andrew Chow)
1f798fe85b wallet: Cache SigningProviders (Andrew Chow)
8a105ecd1a wallet: Use CalculateMaximumSignedInputSize to indicate solvability (Andrew Chow)
Pull request description:
While running my coin selection simulations, I noticed that towards the end of the simulation, the wallet would become slow to make new transactions. The wallet generally performs much more slowly when there are a large number of transactions and/or a large number of keys. The improvements here are focused on wallets with a large number of transactions as that is what the simulations produce.
Most of the slowdown I observed was due to `DescriptorScriptPubKeyMan::GetSigningProvider` re-deriving keys every time it is called. To avoid this, it will now cache the `SigningProvider` produced so that repeatedly fetching the `SigningProvider` for the same script will not result in the same key being derived over and over. This has a side effect of making the function non-const, which makes a lot of other functions non-const as well. This helps with wallets with lots of address reuse (as my coin selection simulations are), but not if addresses are not reused as keys will end up needing to be derived the first time `GetSigningProvider` is called for a script.
The `GetSigningProvider` problem was also exacerbated by unnecessarily fetching a `SigningProvider` for the same script multiple times. A `SigningProvider` is retrieved to be used inside of `IsSolvable`. A few lines later, we use `GetTxSpendSize` which fetches a `SigningProvider` and then calls `CalculateMaximumSignedInputSize`. We can avoid a second call to `GetSigningProvider` by using `CalculateMaximumSignedInputSize` directly with the `SigningProvider` already retrieved for `IsSolvable`.
There is an additional slowdown where `ProduceSignature` with a dummy signer is called twice for each output. The first time is `IsSolvable` checks that `ProduceSignature` succeeds, thereby informing whether we have solving data. The second is `CalculateMaximumSignedInputSize` which returns -1 if `ProduceSignature` fails, and returns the input size otherwise. We can reduce this to one call of `ProduceSignature` by using `CalculateMaximumSignedInputSize`'s result to set `solvable`.
Lastly, a lot of time is spent looking in `mapWallet` and `mapTxSpends` to determine whether an output is already spent. The performance of these lookups is slightly improved by changing those maps to use `std::unordered_map` and `std::unordered_multimap` respectively.
ACKs for top commit:
Xekyo:
ACK bc886fcb31
furszy:
diff re-reACK bc886fcb
Tree-SHA512: fd710fe1224ef67d2bb83d6ac9e7428d9f76a67f14085915f9d80e1a492d2c51cb912edfcaad1db11c2edf8d2d97eb7ddd95bfb364587fb1f143490fd72c9ec1
db10cf8ae3 rpc/wallet: add simulaterawtransaction RPC (Karl-Johan Alm)
701a64f548 test: add support for Decimal to assert_approx (Karl-Johan Alm)
Pull request description:
(note: this was originally titled "add analyzerawtransaction RPC")
This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.
I originally proposed this to Elements (https://github.com/ElementsProject/elements/pull/1016) and it was suggested that I propose this upstream.
There is an alternative #22776 to instead add this info to `getbalances` when providing an optional transaction as argument.
ACKs for top commit:
jonatack:
ACK db10cf8ae3
achow101:
re-ACK db10cf8ae3
Tree-SHA512: adf222ec7dcdc068d007ae6f465dbc35b692dc7bb2db337be25340ad0c2f9c64cfab4124df23400995c700f41c83c29a2c34812121782c26063b100c7969b89d
This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.
Our RBF policy is different from the rules specified in BIP125. For
example, the BIP does not mention Rule 6, and our Rule 4 uses the
(configurable) incremental relay feerate (distinct from the
minimum relay feerate). Those interested in our policy should refer to
doc/policy/mempool-replacements.md instead. These rules may also
continue to diverge with package RBF and other RBF improvements. Keep
references to the BIP125 signaling wrt sequence numbers, since that is
still correct and widely used. It is helpful to refer to this as "BIP125
signaling" since it is unambiguous and succint, especially if we have
multiple ways to signal replaceability in the future.
The rule numbers in doc/policy/mempool-replacements.md correspond
largely to those of BIP 125, so we can still refer to them like "Rule 5."
For some reason, the primary consumer of getWalletTxs requires the
transactions to be in hash order when it is processing them. std::map
will iterate in hash order so the transactions end up in that order when
placed into the vector. To ensure this order when mapWallet is no longer
ordered, the vector is replaced with a set which will maintain the hash
order.
In order to avoid constantly re-deriving the same keys in
DescriptorScriptPubKeyMan, cache the SigningProviders generated inside
of GetSigningProvider.
Rename `BResult` class to `util::Result` and update the class interface to be
more compatible with `std::optional` and with a full-featured result class
implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for
this change is to update existing `BResult` usages now so they don't have to
change later when more features are added in #25665.
This change makes the following improvements originally implemented in #25665:
- More explicit API. Drops potentially misleading `BResult` constructor that
treats any bilingual string argument as an error. Adds `util::Error`
constructor so it is never ambiguous when a result is being assigned an error
or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return
values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same
operators and methods as `std::optional`. `BResult` had a less familiar
interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj
naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so
naming reflects code organization and it is obvious where the class is coming
from. Drops "B" from name because it is undocumented what it stands for
(bilingual?)
- Has unit tests.
ddddd6913b sort after scripted-diff (MacroFake)
fac812ca83 scripted-diff: Move mempool_args to src/node (MacroFake)
66664384a6 Remove ::g_max_datacarrier_bytes global (MacroFake)
fad0b4fab8 Pass datacarrier setting into IsStandard (MacroFake)
fa2a6b8516 Combine datacarrier globals into one (MacroFake)
fa477d32ee Remove ::GetVirtualTransactionSize() alias (MacroFake)
fa2f6c1a61 Remove ::fIsBareMultisigStd global (MacroFake)
fadc14e4f5 Remove ::dustRelayFee (MacroFake)
fa8a7f01fe Remove ::IsStandardTx(tx, reason) alias (MacroFake)
fa7a9114e5 test: Remove unused cs_main (MacroFake)
fa9cba7afb Remove ::incrementalRelayFee and ::minRelayTxFee globals (MacroFake)
fa148602e6 Remove ::fRequireStandard global (MacroFake)
fa468bdfb6 Return optional error from ApplyArgsManOptions (MacroFake)
Pull request description:
This change is good because:
* It moves module-specific init-logic out of the bloated init.cpp
* It removes a global from validation.cpp and places it into the data structure that needs it (mempool)
ACKs for top commit:
glozow:
re ACK ddddd69
ryanofsky:
Code review ACK ddddd6913b
ariard:
Light Code Review ACK ddddd69
Tree-SHA512: 9de2ce601cfcaa4dfd7d1c92270568895ce8702ccdffb59829fbe9618eab0fd88d738afef33ed66988c66861115e0340e881056bfb71e2aed4af2440bd37eb1e
9e04cfaa76 test: add coverage for wallet inconsistent state during sync (furszy)
77de5c693f wallet: guard and alert about a wallet invalid state during chain sync (furszy)
Pull request description:
Follow-up work to my comment in #25239.
Guarding and alerting the user about a wallet invalid state during chain synchronization.
#### Explanation
if the `AddToWallet` tx write fails, the method returns a wtx `nullptr` without removing the recently added transaction from the wallet's map.
Which makes that `AddToWalletIfInvolvingMe` return false (even when the tx is on the wallet's map already), --> which makes `SyncTransaction` skip the `MarkInputsDirty` call --> which leads to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.
Plus, as we only store the arriving transaction inside `AddToWalletIfInvolvingMe` when we synchronize/scan block/s from the chain and nowhere else, it makes sense to treat the transaction db write error as a runtime error to notify the user about the problem. Otherwise, the user will lose all the not stored transactions after a wallet shutdown (without be able to recover them automatically on the next startup because the chain sync would be above the block where the txs arrived).
Note:
On purpose, the first commit adds test coverage for it. Showing how the wallet can end up in an invalid state. The second commit corrects it with the proposed solution.
ACKs for top commit:
achow101:
re-ACK 9e04cfaa76
jonatack:
ACK 9e04cfaa76
Tree-SHA512: 81f765eca40547d7764833d8ccfae686b67c7728c84271bc00dc51272de643dafc270014079dcc9727b47577ba67b340aeb5f981588b54e69a06abea6958aa96
4bedfd702a refactor: remove unneeded temporaries in node/interfaces, simplify code (Jon Atack)
b27ba169eb refactor: make all NodeImpl/ChainImpl/ExternalSignerImpl members public (Jon Atack)
Pull request description:
- Make all `NodeImpl`, `ChainImpl` and `ExternalSignerImpl` class members `public` (and document why), to be consistent in all the `*Impl` classes in `src/node/interfaces.cpp` and `src/wallet/interfaces.cpp` and to help future reviewers and contributors.
- Remove unneeded temporaries in `NodeImpl` and `ChainImpl` methods in `src/node/interfaces.cpp` and simplify, to make the code easier to read and understand and to improve performance by avoiding unnecessary move operations.
ACKs for top commit:
ryanofsky:
Code review ACK 4bedfd702a. Changes since last review, applying suggested style & simplifiying first commit. Also avoiding another lock in second commit.
Tree-SHA512: 112f7cad5e2838c94c5b79d61328f42fe75fdb97f401ab49eccf696fc2c6a8a0c0ee55ec974c0602acf7423f78bb82e90eb8a0cc531e1d3347f73b7c83685504
ab3c06db1a doc: Release notes for default RBF (Andrew Chow)
61d9149e78 rpc: Default rbf enabled (Andrew Chow)
e3c33637ba wallet: Enable -walletrbf by default (Andrew Chow)
Pull request description:
The GUI currently opts in to RBF by default, but RPCs do not, and `-walletrbf` is default disabled. This PR makes the default in those two places to also opt in.
The last time this was proposed (#9527), the primary objections were the novelty at the time, the inability to bump transactions, and the gui not having the option to disable rbf. In the 5 years since, RBF usage has steadily grown, with ~27% of txs opting in. The GUI has the option to enable/disable RBF, and is also defaulted to having it enabled. And we have the ability to bump RBF'd transactions in both the RPC and the GUI. So I think it makes sense to finally change the default to always opt in to RBF.
ACKs for top commit:
darosior:
reACK ab3c06db1a
aureleoules:
ACK ab3c06db1a.
glozow:
utACK ab3c06db1a
Tree-SHA512: 81b012c5033e270f86a87a6a196ccc549eb54b158eebf88e917cc6621d40d7bdcd1566b602688907dd5d364b95a557b29f97dce869cea512e339588262c027b6
as the classes themselves are private, and to be consistent within all the
*Impl classes in src/node/interfaces.cpp and src/wallet/interfaces.cpp
following this order:
public:
// ... virtual methods ...
// ... nonvirtual helper methods ...
// ... data members ...
and add documentation in src/node/interfaces.cpp and src/wallet/interfaces.cpp
to help future reviewers and contributors.
In AvailableCoins, we need to know whether we can solve for an output.
This was done by using IsSolvable, which just calls ProduceSignature and
produces a dummy signature. However, we already do that in order to get
the size of the input by using CalculateMaximumSignedInputSize. As this
function returns -1 if ProduceSignature fails, we can just remove the
use of IsSolvable and check that input_bytes is not -1 to determine
the solvability of an output.
71d1d13627 test: add unit test for AvailableCoins (josibake)
da03cb41a4 test: functional test for new coin selection logic (josibake)
438e04845b wallet: run coin selection by `OutputType` (josibake)
77b0707206 refactor: use CoinsResult struct in SelectCoins (josibake)
2e67291ca3 refactor: store by OutputType in CoinsResult (josibake)
Pull request description:
# Concept
Following https://github.com/bitcoin/bitcoin/pull/23789, Bitcoin Core wallet will now generate a change address that matches the payment address type. This improves privacy by not revealing which of the outputs is the change at the time of the transaction in scenarios where the input address types differ from the payment address type. However, information about the change can be leaked in a later transaction. This proposal attempts to address that concern.
## Leaking information in a later transaction
Consider the following scenario:
1. Alice has a wallet with bech32 type UTXOs and pays Bob, who gives her a P2SH address
2. Alice's wallet generates a P2SH change output, preserving her privacy in `txid: a`
3. Alice then pays Carol, who gives her a bech32 address
4. Alice's wallet combines the P2SH UTXO with a bech32 UTXO and `txid: b` has two bech32 outputs
From a chain analysis perspective, it is reasonable to infer that the P2SH input in `txid: b` was the change from `txid: a`. To avoid leaking information in this scenario, Alice's wallet should avoid picking the P2SH output and instead fund the transaction with only bech32 Outputs. If the payment to Carol can be funded with just the P2SH output, it should be preferred over the bech32 outputs as this will convert the P2SH UTXO to bech32 UTXOs via the payment and change outputs of the new transaction.
**TLDR;** Avoid mixing output types, spend non-default `OutputTypes` when it is economical to do so.
# Approach
`AvailableCoins` now populates a struct, which makes it easier to access coins by `OutputType`. Coin selection tries to find a funding solution by each output type and chooses the most economical by waste metric. If a solution can't be found without mixing, coin selection runs over the entire wallet, allowing mixing, which is the same as the current behavior.
I've also added a functional test (`test/functional/wallet_avoid_mixing_output_types.py`) and unit test (`src/wallet/test/availablecoins_tests.cpp`.
ACKs for top commit:
achow101:
re-ACK 71d1d13627
aureleoules:
ACK 71d1d13627.
Xekyo:
reACK 71d1d13627 via `git range-diff master 6530d19 71d1d13`
LarryRuane:
ACK 71d1d13627
Tree-SHA512: 2e0716efdae5adf5479446fabc731ae81d595131d3b8bade98b64ba323d0e0c6d964a67f8c14c89c428998bda47993fa924f3cfca1529e2bd49eaa4e31b7e426
fa28d0f3c3 scripted-diff: Replace NullUniValue with UniValue::VNULL (MacroFake)
fa962103e8 fuzz: refactor: Replace NullUniValue with UniValue{} (MacroFake)
Pull request description:
This refactor is needed to disable the (potentially expensive for large json) UniValue copy constructors.
ACKs for top commit:
fanquake:
ACK fa28d0f3c3
Tree-SHA512: 7d4204cce0a6fc4ecda96973de77d15b7e4c7caa3e0e890e1f5b9a4b9ace8b240b1f7565d6ab586e168a5fa1201b6c60a924868ef34d6abfbfd8ab7f0f99fbc7
This is required for removing the UniValue copy constructor.
-BEGIN VERIFY SCRIPT-
sed -i 's/return NullUniValue/return UniValue::VNULL/g' $(git grep -l NullUniValue ':(exclude)src/univalue')
-END VERIFY SCRIPT-
fa23c19750 univalue: Avoid narrowing and verbose int constructors (MacroFake)
fa3a9a1e8d rpc: Select int-UniValue constructor for enum value in upgradewallet RPC (MacroFake)
Pull request description:
As UniValue provides several constructors for integral types, the
compiler is unable to select one if the passed type does not exactly
match. This is unintuitive for developers and forces them to write
verbose and brittle code. (Refer to `-Wnarrowing` compiler warning)
For example, there are many places where an unsigned int is cast to a
signed int. While the cast is safe in practice, it is still needlessly
verbose and confusing as the value can never be negative. In fact it
might even be unsafe if the unsigned value is large enough to map to a
negative signed one.
Fix this issue and other (minor) type issues.
ACKs for top commit:
aureleoules:
ACK fa23c19750.
Tree-SHA512: 7d99b5b90c7d8eed2e3448167255a59e817dd6b8fcfc1b17c69ddefd0db33d1bf4344fbcd8b7f8685b58182c0f572ab9ffa99467afa666ac21843df7ea645033
faf9accd66 Use HashWriter where possible (MacroFake)
faa5425629 Add HashWriter without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `HashWriter`. `CHashWriter` remains in places where it is not yet possible.
ACKs for top commit:
sipa:
utACK faf9accd66
Empact:
utACK faf9accd66
Tree-SHA512: 544cc712436e49f6e608120bcd3ddc5ea72dd236554ce30fb6cfff34a92d7e67b6e6527336ad0f5b6365e2b2884f4c6508aef775953ccd9312f17752729703f2
47ea70fbb8 wallet: clean AllInputsMine code, use InputIsMine internally (furszy)
bf310b0e8c wallet: clean InputIsMine code, use GetWalletTx (furszy)
0cb177263c wallet: unify CachedTxGetImmatureCredit and CachedTxGetImmatureWatchOnlyCredit (furszy)
04c6423f7b wallet: remove always true 'fUseCache' arg from CachedTxGetAvailableCredit (furszy)
4f0ca9bff6 wallet: remove always false 'recalculate' arg from GetCachableAmount (furszy)
47b1012677 wallet: remove always true 'fUseCache' from CachedTxGetImmatureWatchOnlyCredit (furszy)
da8f62de2c wallet: remove always true 'fUseCache' from CachedTxGetImmatureCredit (furszy)
Pull request description:
Another wallet's code garbage collector work. Part of the `mapWallet` encapsulation goal.
Focused on the following points:
1) Remove always true `fUseCache` argument from `CachedTxGetImmatureCredit`, `CachedTxGetImmatureWatchOnly` and `CachedTxGetAvailableCredit`.
2) Remove always false `recalculate` argument from `GetCachableAmount`.
3) Merge `CachedTxGetImmatureCredit` and `CachedTxGetImmatureWatchOnlyCredit` as they do share the exact same code.
4) Clean `InputIsMine` method; use `GetWalletTx` instead of access the wallet's map directly.
5) Clean `AllInputsMine` method; use `InputIsMine` instead of duplicate the exact same code internally.
ACKs for top commit:
aureleoules:
re-ACK 47ea70fbb8
achow101:
ACK 47ea70fbb8
theStack:
re-ACK 47ea70fbb8
Tree-SHA512: e9b64b57de7be6165c5e5552e28cd8a03d4736b0a3707d29d129e3a0a3db6a855c2abf47a24917236060835a297b564a97b66d4c8b178d6bdafb93a12a7c0b40
