Instead of having an entire TaprootBuilder which may or may not be
complete, and could potentially have future changes that interact oddly
with taproot tree tuples, have m_tap_tree be just the tuples.
When needed in other a TaprootBuilder for actual use, the tuples will be
added to a a TaprootBuilder that, in the future, can take in whatever
other data is needed as well.
The test checks that parent txs are broadcast before child txs.
The previous behavior is that the rebroadcasting would simply iterate mapWallet. As
mapWallet is a std::unsorted_map, the child can sometimes come before the parent and thus
be rebroadcast in the wrong order and fail the test.
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.
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.
e90a445d7e scripted-diff: rpc: fix rescan RPC name (s/rescanwallet/rescanblockchain/) (Sebastian Falbesoner)
Pull request description:
There is no RPC call named `rescanwallet`, i.e. fix this by renaming to the actual RPC called `rescanblockchain`.
ACKs for top commit:
achow101:
ACK e90a445d7e
aureleoules:
ACK e90a445d7e.
promag:
ACK e90a445d7e
Tree-SHA512: abf1d1c18de32d87c29e4ff2b782dfb0e4a46dc2c2cc51ab616d12674a0f4a5d22214e00955663ae897cbb88f4f6ced913850f28ea3f5c1b3a54577a25fbf399
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 https://github.com/bitcoin/bitcoin/pull/25784/commits/0cb6d2aec63aec76a517b8da621a3c53ab432632
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.
416ceb8661 descriptor: check if `rawtr` has only one key. (w0xlt)
Pull request description:
If I understand `rawtr` descriptor correctly, it should only allow `rawtr(KEY)`, not `rawtr(KEY1, KEY2, ...)` or other concatenations.
On master branch, `rawtr(KEY1, KEY2, ...)` will produce the `rawtr(KEY1)` descriptor ignoring the `KEY2, ...` with no error messages or warnings.
For example, the code below will print `rawtr(tprv8ZgxMBicQKsPefef2Doobbq3xTCaVTHcDn6me82KSXY1vY9AJAWD5u7SDM4XGLfc4EoXRMFrJKpp6HNmQWA3FTMRQeEmMJYJ9RPqe9ne2hU/*)#lx9qryfh`
for the supposedly invalid descriptor
`rawtr(tprv8ZgxMBicQKsPefef2Doobbq3xTCaVTHcDn6me82KSXY1vY9AJAWD5u7SDM4XGLfc4EoXRMFrJKpp6HNmQWA3FTMRQeEmMJYJ9RPqe9ne2hU/*, tprv8ZgxMBicQKsPezQ2KGArMRovTEbCGxaLgBgaVcTvEx8mby8ogX2bgC4HBapH4yMwrz2FpoCuA17eocuUVMgEP6fnm83YpwSDTFrumw42bny/*)`
```python
self.nodes[1].createwallet(wallet_name="rawtr_multi", descriptors=True, blank=True)
rawtr_multi = self.nodes[1].get_wallet_rpc("rawtr_multi")
rawtr_multi_desc = "rawtr(tprv8ZgxMBicQKsPefef2Doobbq3xTCaVTHcDn6me82KSXY1vY9AJAWD5u7SDM4XGLfc4EoXRMFrJKpp6HNmQWA3FTMRQeEmMJYJ9RPqe9ne2hU/*, tprv8ZgxMBicQKsPezQ2KGArMRovTEbCGxaLgBgaVcTvEx8mby8ogX2bgC4HBapH4yMwrz2FpoCuA17eocuUVMgEP6fnm83YpwSDTFrumw42bny/*)#uv78hkt0"
result = rawtr_multi.importdescriptors([{"desc": rawtr_multi_desc, "active": True, "timestamp": "now"}])
print(rawtr_multi.listdescriptors(True))
```
This PR adds a check that prevents `rawtr` descriptors from being created if more than one key is entered, shows an error message, and adds a test for this case.
ACKs for top commit:
achow101:
ACK 416ceb8661
sipa:
ACK 416ceb8661
Tree-SHA512: a2009e91f1bca6ee79cc68f65811caa6a21fc8b80acd8dc58e283f424b41fe53b0db7ce3693b1c7e2184ff571e6d1fbb9f5ccde89b65d3026726f3393c492044
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 https://github.com/bitcoin/bitcoin/pull/25504/commits/a6b0c1fcc06485ecd320727fa7534a51a20608c1
achow101:
re-ACK a6b0c1fcc0
Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37
f6a916683d Add functional test for block announcements during initial headers sync (Suhas Daftuar)
05f7f31598 Reduce bandwidth during initial headers sync when a block is found (Suhas Daftuar)
Pull request description:
On startup, if our headers chain is more than a day behind current time, we'll pick one peer to sync headers with until our best headers chain is caught up (at that point, we'll try to sync headers with all peers).
However, if an INV for a block is received before our headers chain is caught up, we'll then start to sync headers from each peer announcing the block. This can result in doing a big headers sync with many (if not all) of our peers simultaneously, which wastes bandwidth.
This PR would reduce that overhead by picking (at most) one new peer to try syncing headers with whenever a new block is announced, prior to our headers chain being caught up.
ACKs for top commit:
LarryRuane:
ACK f6a916683d
ajtowns:
ACK f6a916683d
mzumsande:
ACK f6a916683d
dergoegge:
Code review ACK f6a916683d
achow101:
ACK f6a916683d
Tree-SHA512: 0662000bd68db146f55981de4adc2e2b07cbfda222b1176569d61c22055e5556752ffd648426f69687ed1cc203105515e7304c12b915d6270df8e41a4a0e1eaa
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 https://github.com/bitcoin/bitcoin/pull/25664/commits/b16f93caddcd3254eaf3dc43e09adf2142a9c40a
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 https://github.com/bitcoin/bitcoin/pull/25642/commits/fb9faffae3a26b8aed8b671864ba679747163019
Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343
544b4332f0 Add wallet tests for spending rawtr() (Pieter Wuille)
e1e3081200 If P2TR tweaked key is available, sign with it (Pieter Wuille)
8d9670ccb7 Add rawtr() descriptor for P2TR with unknown tweak (Pieter Wuille)
Pull request description:
It may be useful to be able to represent P2TR outputs in descriptors whose script tree and/or internal key aren't known. This PR does that, by adding a `rawtr(KEY)` descriptor, where the KEY represents the output key directly. If the private key corresponding to that output key is known, it also permits signing with it.
I'm not convinced this is desirable, but presumably "tr(KEY)" sounds more intended for direct use than "rawtr(KEY)".
ACKs for top commit:
achow101:
ACK 544b4332f0
sanket1729:
code review ACK 544b4332f0
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/23480/commits/544b4332f0e122167bdb94dc963405422faa30cb
Tree-SHA512: 0de08de517468bc22ab0c00db471ce33144f5dc211ebc2974c6ea95709f44e830532ec5cdb0128c572513d352120bd651c4559516d4500b5b0a3d257c4b45aca
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
7a0b129c41 guix: patch NSIS to remove .reloc sections from install stubs (fanquake)
Pull request description:
With the release of binutils/ld 2.36, ld swapped to much improved
default settings when producing windows binaries with mingw-w64. One of
these changes was to stop stripping the .reloc section from binaries,
which is required for working ASLR.
When we switched to using a newer Guix time-machine in #23778, we begun
using binutils 2.37 to produce releases. Since then, our windows
installer (produced with makensis) has not functioned correctly when run on
a Windows system with the "Force randomization for images (Mandatory ASLR)"
option enabled. Note that all of our other release binaries, which all
contain .reloc sections, function fine under the same option, so it
cannot be just the presence of a .reloc section that is the issue.
The root cause of the problem is that when we compile NSIS (makensis), a number
of exe installer stubs are produced at the same time, for use later when makensis
is actually run. Given the new linker defaults, the stubs will contain .reloc sections,
when previously they would not. It seems that, in combination with how makensis
mutates the stub when it actually builds the installer, causes the problem.
According to upstream, https://sourceforge.net/p/nsis/bugs/1131/#abb6:
> Looks like the problem is the very existance of the .reloc section.
> It's not supposed to be there, and makensis doesn't handle it.
The most recent .reloc related upstream activity is in
https://sourceforge.net/p/nsis/bugs/1283/, where the conclusion again seemed to
be that .relo sections are not wanted, but there hasn't been any further follow up.
For now, restore pre-binutils-2.36 behaviour, by passing `-Wl,--disable-reloc-section`
to the linker when building the installer stubs, which fixes the produced installer.
The underlying issue can be further investigated in future.
.reloc section stripping is something we've accounted for previously,
see #18702, and related upstream discussion is in this thread:
https://sourceware.org/bugzilla/show_bug.cgi?id=19011.
Fixes#25726.
Guix Build (x86_64):
```bash
7e0723388913ac1ec9f650b943c6b23351ba0cd921c0ec830abf16b16724d503 guix-build-7a0b129c41d9/output/dist-archive/bitcoin-7a0b129c41d9.tar.gz
c3bb9c68895ffafa2900b0d18c1268e299d012a7dc70593f20f9900cf116eb05 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/SHA256SUMS.part
b57aa99c242b0aae64653c64ada38f6d3f0cbd902bbc096d3dc529fdcf87d681 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-debug.zip
341d99afc9961299883be6cd9666e8bc0f3f6296cff758719a32d27419acad36 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-setup-unsigned.exe
1d9ef48d3c9ed93a925962356b41cdaeb9d09fd758de193cd4d5f4d1ec6791eb guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-unsigned.tar.gz
28c81d99a9a4bd6648449393f91db213369e958add579ba9e9a1721540d2c4f7 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64.zip
```
Guix Build (arm64):
```bash
7e0723388913ac1ec9f650b943c6b23351ba0cd921c0ec830abf16b16724d503 guix-build-7a0b129c41d9/output/dist-archive/bitcoin-7a0b129c41d9.tar.gz
c3bb9c68895ffafa2900b0d18c1268e299d012a7dc70593f20f9900cf116eb05 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/SHA256SUMS.part
b57aa99c242b0aae64653c64ada38f6d3f0cbd902bbc096d3dc529fdcf87d681 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-debug.zip
341d99afc9961299883be6cd9666e8bc0f3f6296cff758719a32d27419acad36 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-setup-unsigned.exe
1d9ef48d3c9ed93a925962356b41cdaeb9d09fd758de193cd4d5f4d1ec6791eb guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-unsigned.tar.gz
28c81d99a9a4bd6648449393f91db213369e958add579ba9e9a1721540d2c4f7 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64.zip
```
ACKs for top commit:
achow101:
ACK 7a0b129c41
hebasto:
ACK 7a0b129c41
jarolrod:
ACK 7a0b129c41
Tree-SHA512: 9e14e98207d20236b833603319fc4bb335c878a7c179ab495b33d143e2a900c6926125536bbb7499ee4f0f676cd5ea45c8c86cd7e544ed9a76bb298f98db6197
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
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.
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
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
4fa79837ad psbt: Fix unsigned integer overflow (Aurèle Oulès)
Pull request description:
Fixes#25692.
This change prevents an unsigned integer overflow during the deserialization of a PSBT.
ACKs for top commit:
achow101:
ACK 4fa79837ad
Tree-SHA512: 0863d4d31ada1ba50632b6a66cb4c694c0a15680a90cf9370129cf3db15e3c10e65610b779db047d5a4cc7c920708b728948708e4023e916099c6bfe730f01f9
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
71a751f6c3 test: add test for decoding PSBT with per-input preimage types (Sebastian Falbesoner)
faf43378e2 refactor: move helper `random_bytes` to util library (Sebastian Falbesoner)
fdc1ca3896 test: add constants for PSBT key types (BIP 174) (Sebastian Falbesoner)
1b035c03f9 refactor: move PSBT(Map) helpers from signet miner to test framework (Sebastian Falbesoner)
7c0dfec2dd refactor: move `from_binary` helper from signet miner to test framework (Sebastian Falbesoner)
597a4b35f6 scripted-diff: rename `FromBinary` helper to `from_binary` (signet miner) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `decodepsbt` RPC in the case that a PSBT with on of the per-input preimage types (`PSBT_IN_RIPEMD160`, `PSBT_IN_SHA256`, `PSBT_IN_HASH160`, `PSBT_IN_HASH256`; see [BIP 174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#Specification)) is passed. As preparation, the first four commits move the already existing helpers for (de)serialization of PSBTs and PSBTMaps from the signet miner to the test framework (in a new module `psbt.py`), which should be quite useful for further tests to easily create PSBTs.
ACKs for top commit:
achow101:
ACK 71a751f6c3
Tree-SHA512: 04f2671612d94029da2ac8dc15ff93c4c8fcb73fe0b8cf5970509208564df1f5e32319b53ae998dd6e544d37637a9b75609f27a3685da51f603f6ed0555635fb
c32fa85909 depends: modify FastFixedDtoa optimisation flags (fanquake)
Pull request description:
This fixes a non-determinism issue in the asm produced for
this function when cross-compiling on x86_64 and aarch64 for
the arm-linux-gnueabihf HOST.
Related to #21194. Alternative to #25636. Initial discussion in https://github.com/bitcoin/bitcoin/pull/24615#issuecomment-1080809879.
Guix Build (x86_64):
```bash
28ae0ec2874ead334edd1c5dc509344379d82f7058b649c9076992defd7190d7 guix-build-c32fa85909dd/output/arm-linux-gnueabihf/SHA256SUMS.part
48d34073b029c4f62c8e1bd906533610228d5ca0bb5eefea6010dfa7372ba067 guix-build-c32fa85909dd/output/arm-linux-gnueabihf/bitcoin-c32fa85909dd-arm-linux-gnueabihf-debug.tar.gz
850d6e9859e88bcb93ed586bdb0c0bc95a44249d6a0ec1b1d13125cb9dd56413 guix-build-c32fa85909dd/output/arm-linux-gnueabihf/bitcoin-c32fa85909dd-arm-linux-gnueabihf.tar.gz
b8bb092b1307684ea4b53d810ac110ec14f29eeab8028d924d1cac7418009b14 guix-build-c32fa85909dd/output/dist-archive/bitcoin-c32fa85909dd.tar.gz
```
Guix Build (arm64):
```bash
28ae0ec2874ead334edd1c5dc509344379d82f7058b649c9076992defd7190d7 guix-build-c32fa85909dd/output/arm-linux-gnueabihf/SHA256SUMS.part
48d34073b029c4f62c8e1bd906533610228d5ca0bb5eefea6010dfa7372ba067 guix-build-c32fa85909dd/output/arm-linux-gnueabihf/bitcoin-c32fa85909dd-arm-linux-gnueabihf-debug.tar.gz
850d6e9859e88bcb93ed586bdb0c0bc95a44249d6a0ec1b1d13125cb9dd56413 guix-build-c32fa85909dd/output/arm-linux-gnueabihf/bitcoin-c32fa85909dd-arm-linux-gnueabihf.tar.gz
b8bb092b1307684ea4b53d810ac110ec14f29eeab8028d924d1cac7418009b14 guix-build-c32fa85909dd/output/dist-archive/bitcoin-c32fa85909dd.tar.gz
```
ACKs for top commit:
achow101:
ACK c32fa85909
hebasto:
ACK c32fa85909
jarolrod:
ACK c32fa85909
Tree-SHA512: 137d76274b1421247f43e5f040b4b5c42473f94d734498c73ab40e580c47dfecbbf11f1a69c15a87d805d4b8e9ef1fd62cc1b69c0f1614c62ff3cba98b1733e8
817326a828 wallet: avoid rescans if under the snapshot (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Refuse to load a wallet if it requires a rescan lower than the height of assumed-valid blocks.
Of course in live code right now, `BLOCK_ASSUMED_VALID` block index entries don't exist since they're a unique flag introduced by the use of UTXO snapshots, so this is prophylactic code exercised only by unittests.
ACKs for top commit:
achow101:
ACK 817326a828
ryanofsky:
Code review ACK 817326a828. This seems like the simplest change we can make to avoid wallet problems when an assumeutxo snapshot is loaded.
Tree-SHA512: cfa44b2eb33d1818d30df45210d0dde1e9b78cc9b7c88cb985054dc28427bba9e0905debe4196065d1d3a5ce7bca7e605e629d5ce5f0225b25395746e6d3d596
1be7964189 test, wallet: Add mempool rescan test for import RPCs (Fabian Jahr)
833ce76df7 rpc, wallet: Document mempool rescan after importdescriptor, importwallet (Fabian Jahr)
0e396d1ba7 rpc, wallet: Document mempool scan after importmulti (Fabian Jahr)
e6d3ef8586 rpc, wallet: Document mempool scan after importpubkey (Fabian Jahr)
6d3db52e66 rpc, wallet: Document and test mempool scan after importprivkey (João Barbosa)
3abdbbb90a rpc, wallet: Document and test mempool scan after importaddress (João Barbosa)
236239bd40 wallet: Rescan mempool for transactions as well (Fabian Jahr)
Pull request description:
This PR picks up the work from #18964 and closes#18954.
It should incorporate all the unaddressed feedback from the PR:
- Mempool rescan now expanded to all relevant import* RPCs
- Added documentation in the help of each RPC
- More tests
ACKs for top commit:
Sjors:
re-utACK 1be7964189 (only a test change)
achow101:
ACK 1be7964189
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/25351/commits/1be796418934ae7370cb0ed501877db59e738106
Tree-SHA512: b62fed5f97c6c242b2af417b41c9696a1f18878483d9e1c9429791f9c05257f57a00540a9a84df23c49faf6a61c3109c22972de81540083f38b506217804fcc5
Our usage of std::atomic is with it's own exchange function, not
std::atomic_exchange. So we should be looking specifically for that
function.
Additionally, -pthread and -lpthread have an effect on whether -latomic
will be needed, so the atomics check needs to use these flags as well.
This will make the flags in use better match what is actually used when
linking.
This removes the need for -latomic for riscv builds, which resolves a
guix cross architecture reproducibility issue.
Watchonly wallets do not have any private keys to encrypt. It does not
make sense to encrypt such wallets, so disable the option to encrypt
them.
This avoids an assertion that can be hit when encrypting watchonly descriptor
wallets.
ffc79b8e49 qa: functional test Miniscript watchonly support (Antoine Poinsot)
bfb036756a Miniscript support in output descriptors (Antoine Poinsot)
4a082887be qa: better error reporting on descriptor parsing error (Antoine Poinsot)
d25d58bf5f miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot)
c38c7c5817 miniscript: don't check for top level validity at parsing time (Antoine Poinsot)
Pull request description:
This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of #24147 for a description of Miniscript and a rationale of having it in Bitcoin Core.
On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support.
A minified corpus of Miniscript Descriptors for the `descriptor_parse` fuzz target is available at https://github.com/bitcoin-core/qa-assets/pull/92.
The Miniscript descriptors used in the unit tests here and in #24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript.
This PR contains code and insights from Pieter Wuille.
ACKs for top commit:
Sjors:
re-utACK ffc79b8e49
achow101:
ACK ffc79b8e49
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/24148/commits/ffc79b8e492c6dd1352e528fd82e45d8d25eaa04
Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9
If we are given a PSBT that is missing one or more input UTXOs, our
PrecomputedTransactionData will be incorrect and missing information
that it should otherwise have, and therefore we may not produce a
signature when we should. To avoid this problem, we can do the
precomputation after we have set the UTXOs the wallet is able to set for
the PSBT.
Also adds a test for this behavior.
98ea43d5e9 test: add tests for negative waste during coin selection (ishaanam)
Pull request description:
#25495 mentions that waste can be negative when the current feerate is less than the long term feerate. There are currently no waste tests for negative waste, so this PR adds two of them.
ACKs for top commit:
achow101:
ACK 98ea43d5e9
glozow:
light code review ACK 98ea43d5e9, good to have tests for negative waste
Tree-SHA512: d194d370f1257975959d3c601fea9f82c30c1aabc3e8bedc997c62659283fe681cc527e59df1a0187b3c91e8067c60374dd5ce0237561bd882edafe6a575a9b9
d54c5c8b1b wallet: use CCoinControl to estimate signature size (S3RK)
a94659c84e wallet: replace GetTxSpendSize with CalculateMaximumSignedInputSize (S3RK)
Pull request description:
Currently `DummySignTx` and `DummySignInput` use different ways to determine signature size.
This PR unifies the way wallet estimates signature size for various inputs.
Instead of passing boolean flags from calling code the `use_max_sig` is now calculated at the place of signature creation using information available in `CCoinControl`
ACKs for top commit:
achow101:
ACK d54c5c8b1b
theStack:
Code-review ACK d54c5c8b1b
Tree-SHA512: e790903ad4683067070aa7dbf7434a1bd142282a5bc425112e64d88d27559f1a2cd60c68d6022feaf6b845237035cb18ece10f6243d719ba28173b69bd99110a
d69045e291 test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy)
324f00a642 refactor: 'ListReceived' use optional for filtered address (furszy)
b459fc122f refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy)
fa9f2ab8fd refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy)
83e42c4b94 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy)
2b48642499 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy)
032842ae41 wallet: implement ForEachAddrBookEntry method (furszy)
09649bc95d refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy)
192eb1e61c refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy)
Pull request description:
### Context
The wallet's `m_address_book` field is being accessed directly from several places across the sources.
### Problem
Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own.
### Solution
Encapsulate `m_address_book` access inside the wallet.
-------------------------------------------------------
Extra Note:
This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR).
ACKs for top commit:
achow101:
ACK d69045e291
theStack:
ACK d69045e291✅
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/25337/commits/d69045e291e32e02d105d1b5ff1c8b86db0ae69e
Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
140d942634 wallet: don't add change fee to target if subtracting fees from output (S3RK)
Pull request description:
Change fee is payed by the recipient, so we don't need to add it to our target for coin selection.
ACKs for top commit:
achow101:
ACK 140d942634
ishaanam:
ACK 140d942634
furszy:
Code review ACK 140d9426
Tree-SHA512: b5efd0264c47ecee9204a3fd039bad24c69f9e614c6e1d9bb240ee5be6356b175aa074f3be123e6cfb8becd4d7bd1028eebe18801662cc69d19413d8d5a9dd5c
Since TaprootBuilder has assertions for the depth and leaf versions, the
PSBT decoder should check these values before calling
TaprootBuilder::Add so that the assertions are not triggered on
malformed taproot trees.
28a28a0c5b Squashed 'src/minisketch/' changes from 7eeb778fef..47f0a2d26f (fanquake)
Pull request description:
Contains:
* https://github.com/sipa/minisketch/pull/65
* https://github.com/sipa/minisketch/pull/66
Required for #25493.
ACKs for top commit:
achow101:
ACK dc375e5cce
hebasto:
ACK dc375e5cce, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: fbcd6cdc551770ff67d1df65ab171ce43c9eb7e7668da76da5c5b06865ed550527abcff661741a86c1535018a85a165619ff94ae3e6c7a695374b6c4f843c5ca
25e4762ae7 wallet: more accurate tx_noinputs_size (S3RK)
Pull request description:
Rationale: more accurate non-input fee estimation for txs with >=253 inputs
ACKs for top commit:
laanwj:
Concept and code review ACK 25e4762ae7
achow101:
ACK 25e4762ae7
furszy:
Code review ACK 25e4762a. left a small nit.
Tree-SHA512: bda8fad725d32ad3e13c007fa56ddb6679ac1a32098ddb08d9a114054acfa681cb66cd703ac675297f731cb381b09067a99a4efa31320140bbdd03f0cfdc81af
Without ffile-prefix-map, the debug symbols will contain paths for the
guix store which will include the hashes of each package. However, the
hash for the same package will differ when on different architectures.
In order to be reproducible regardless of the architecture used to build
the package, map all guix store prefixes to something fixed, e.g. /usr.
We might be able to drop this in favour of using --with-nonshared-cflags
when we being using newer versions of glibc.
Without ffile-prefix-map, the debug symbols will contain paths for the
guix store which will include the hashes of each package. However, the
hash for the same package will differ when on different architectures.
In order to be reproducible regardless of the architecture used to build
the package, map all guix store prefixes to something fixed, e.g. /usr.
When filling a PSBT, we search the listed pubkeys in order to determine
whether the current DescriptorScriptPubKeyMan could sign the transaction
even if it is not watching the scripts. With Taproot, the taproot
pubkeys need to be searched as well.
The taproot spenddata stored in a sigdata is the combination of data
existing previously (e.g. in a PSBT) and the data stored in a
SigningProvider. In order to use the external data when signing, we need
to be using the sigdata's spenddata.
GetSpendData needs to be finalized in order to be used. To avoid future
bugs, assert `!m_output_key.IsNull()` as m_output_key is only set during
Finalize.
TaprootSpendData can be gotten from TaprootBuilder, however for PSBT, we
also need TaprootBuilders directly (for the outputs). So we store the
TaprootBuilder in the FlatSigningProvider and when the TaprootSpendData
is needed, we generate it on the fly using the stored builder.
It is useful to have serialzation methods for XOnlyPubKey. These will
serialize the internal uint256, so it is not prefixed with the length as
CPubKey does.
GetTreeTuples returns the leaves in DFS order as tuples of depth, leaf
version, and script. This is a representation of the tree that can be
serialized.
To make it easier to de/serialize individual KeyOriginInfo for PSBTs,
separate the actual de/serialization of KeyOriginInfo to its own
function.
This is an additional separation where any length prefix is processed by
the caller.
32e5edc0f4 wallet: avoid extra wtx lookup in AddToSpends (furszy)
Pull request description:
As `AddToSpends` is only called from `AddToWallet` and `LoadToWallet`, places where we insert the wtx into the wallet map, we can directly feed `AddToSpends` with the `wtx` and remove another extra lookup.
ACKs for top commit:
laanwj:
Code review ACK 32e5edc0f4
achow101:
ACK 32e5edc0f4
theStack:
Code-review ACK 32e5edc0f4
w0xlt:
Code Review ACK https://github.com/bitcoin/bitcoin/pull/25427/commits/32e5edc0f454c59c8e0d8d86a9abfa9a3f25ca28
brunoerg:
crACK 32e5edc0f4
Tree-SHA512: e9fb8df44c3e3fa26c107d261bf78e45014b4755890a64817f2be62ee6b7751f5dd2813a18dcb103a21ddba1422f9d2d59c4bf186f08314e634365d36b01be8f
fd5c996d16 wallet: GetAvailableBalance, remove double walk-through every available coin (furszy)
162d4ad10f wallet: add 'only_spendable' filter to AvailableCoins (furszy)
cdf185ccfb wallet: remove unused IsSpentKey(hash, index) method (furszy)
4b83bf8dbc wallet: avoid extra IsSpentKey -> GetWalletTx lookups (furszy)
3d8a282257 wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n) (furszy)
a06fa94ff8 wallet: IsSpent, 'COutPoint' arg instead of (hash, index) (furszy)
91902b7720 wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) (furszy)
9472ca0a65 wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times (furszy)
4ce235ef8f wallet: return 'CoinsResult' struct in `AvailableCoins` (furszy)
Pull request description:
This started in #24845 but grew out of scope of it.
So, points tackled:
1) Avoid extra `GetWalletTx` lookups inside `AvailableCoins -> IsSpentKey`.
`IsSpentKey` was receiving the tx hash and index to internally lookup the tx inside the wallet's map. As all the `IsSpentKey` function callers already have the wtx available, them can provide the `scriptPubKey` directly.
2) Most of the time, we call `Wallet::AvailableCoins`, and later on the process, skip the non-spendable coins from the result in subsequent for-loops. So to speedup the process: introduced the ability to filter by "only_spendable" coins inside `Wallet::AvailableCoins` directly.
(the non-spendable coins skip examples are inside `AttemptSelection->GroupOutputs` and `GetAvailableBalance`).
4) Refactored `AvailableCoins` in several ways:
a) Now it will return a new struct `CoinsResult` instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). --> this is coming from #24845 but cherry-picked it here too to make the following commits look nicer.
b) Unified all the 'wtx.tx->vout[I]' calls into a single call (coming from this comment https://github.com/bitcoin/bitcoin/pull/24699#discussion_r854163032).
5) The wallet `IsLockedCoin` and `IsSpent` methods now accept an `OutPoint` instead of a hash:index. Which let me cleanup a bunch of extra code.
6) Speeded up the wallet 'GetAvailableBalance': filtering `AvailableCoins` by spendable outputs only and using the 'AvailableCoins' retrieved `total_amount` instead of looping over all the retrieved coins once more.
-------------------------------------------------------
Side topic, all this process will look even nicer with #25218
ACKs for top commit:
achow101:
ACK fd5c996d16
brunoerg:
crACK fd5c996d16
w0xlt:
Code Review ACK https://github.com/bitcoin/bitcoin/pull/25005/commits/fd5c996d1609e6f88769f6f3ef0c322e3435b3aa
Tree-SHA512: 376a85476f907f4f7d1fc3de74b3dbe159b8cc24687374d8739711ad202ea07a33e86f4e66dece836da3ae6985147119fe584f6e672f11d0450ba6bd165b3220
7832e9438f test: fundrawtransaction preset input weight calculation (S3RK)
c3981e379f wallet: do not count wallet utxos as external (S3RK)
Pull request description:
Correctly differentiating between external vs non-external utxos in coin control produces more accurate weight and fee estimations.
Weight for external utxos is estimated based on the maximum signature size, while for the wallet utxos we expect minimal signature due to signature grinding.
ACKs for top commit:
achow101:
re-ACK 7832e9438f
Xekyo:
re-ACK 7832e9438f
furszy:
ACK 7832e943
Tree-SHA512: bb5635b0bd85fa9a76922a53ad3fa062286424c06a695a0e87407c665713e80a33555b644fbb13bcc1ab503dcd7f53aacbdc368d69ac0ecff8005603623ac94f
e3609cdc01 doc: Update importaddress mention incompatibility with descriptor wallet (BrokenProgrammer)
Pull request description:
This is related to #25363 and offers a small update to the error messages from `EnsureLegacyScriptPubKeyMan` and `EnsureConstLegacyScriptPubKeyMan` to mention that they only are compatible with legacy wallets.
The RPC documentation for `importaddress` is also updated to mention this as well as guide the user to the alternative `importdescriptors` for cases when using descriptor wallets.
I'm thinking that we can introduce a "porting guide" document mentioned in #25363 in a separate PR since I would have to make myself more familiar with the subject before being able to tackle that.
ACKs for top commit:
laanwj:
Code review ACK e3609cdc01
achow101:
ACK e3609cdc01
Tree-SHA512: c7a924a7283fe59dc4e04c8c8fa034c15601f0b25eff09d975e98e2e8db5268ff470336b2d978d6916af9f782f9257b840d64bd15485b1742b4a8b8bfd0bb50f
57fb37c275 wallet: CommitTransaction, remove extra wtx lookup and add exception for a possible db write error. (furszy)
Pull request description:
Two points for `CWallet::CommitTransaction`:
1) The extra wtx lookup:
As we are calling to `AddToWallet` first, which returns the recently added/updated wtx pointer, there is no need to look up the wtx again few lines later. We can just use it.
2) The db write error:
`AddToWallet` can only return a nullptr if the db write fails, which inside `CommitTransaction` translates to an exception throw cause. We expect everywhere that `CommitTransaction` always succeed.
------------------------------------------------
Extra note:
This finding generated another working path for me :)
It starts with the following question: why are we returning a nullptr from `AddToWallet` if the db write failed without removing the recently added transaction from the wallet's map?..
Can led 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.
-- I'm writing it here to gather some feedback first and not forget it, will create a follow-up PR in the coming days 🚜 --
ACKs for top commit:
achow101:
ACK 57fb37c275
jonatack:
ACK 57fb37c
ryanofsky:
Code review ACK 57fb37c275. Seems like a clear improvement. Better to fail earlier with a better error message if the failure is going to happen anyway
Tree-SHA512: 80e59c01852cfbbc70a5de1a1c2c59b5e572f9eaa08c2175112cb515256e63fa04c7942f92a513b620d6b06e66392029ebe8902287c456efdbee58a7a5ae42da
6b636730f4 tracing: fix `coin_selection:aps_create_tx_internal` calling logic (Sebastian Falbesoner)
Pull request description:
According to the documentation, the tracepoint `coin_selection:aps_create_tx_internal` "Is called when the second `CreateTransactionInternal` with Avoid Partial Spends enabled completes."
Currently it is only called if the second call to `CreateTransactionInternal` succeeds, i.e. the third parameter is always `true` and we don't get notified in the case that it fails. This PR fixes this by moving the tracepoint call and the `use_aps` boolean variable outside the if body.
ACKs for top commit:
achow101:
ACK 6b636730f4
furszy:
re-ACK 6b636730
Tree-SHA512: 453825123aa10748642c7dd94324ced2d07df0f4fac478b0947a34820b515ae300f75721679a90a164f3127029739df55c4de035c4567e663893c3c6dbdef216
Andrew Chow
