c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 wallet, rpc, test: Remove deprecated getunconfirmedbalance (Ava Chow)
0ec255139be3745a135386e9db957fe81bc3d833 wallet, rpc: Remove deprecated balances from getwalletinfo (Ava Chow)
Pull request description:
`getwalletinfo` result fields `balance`, `immature_balance`, and `unconfirmed_balance`, and the `getunconfirmedbalance` RPC have all been deprecated since 0.19.0. It's been long enough that they should either be removed or undeprecated. The functionality provided by these RPCs is provided by `getbalances`.
ACKs for top commit:
davidgumberg:
ACK c3fe85e2d6
rkrux:
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
BrandonOdiwuor:
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
w0xlt:
reACK c3fe85e2d6
Tree-SHA512: c7c4acfd9cabc7517ba813b95281a6c6a717a417312afd9346298669b4f7bd37724ad977148ce42db7fd47fc3d1f5a8482d8ff2e7b9cb74756b171a5b8b91ef2
47237cd1938058b29fdec242c3a37611e255fda0 wallet, rpc: Output wallet flags in getwalletinfo (Ava Chow)
bc2a26b296238cbead6012c071bc7741c40fbd02 wallet: Add GetWalletFlags (Ava Chow)
69f588a99a7a79d1d72300bc0f2c8475f95f6c6a wallet: Set upgraded descriptor cache flag for newly created wallets (Ava Chow)
Pull request description:
Newly created wallets will always have an upgraded descriptor cache, so set those.
Also, to verify this behavior, add a new `flags` field to `getwalletinfo` and check that in the functional tests.
Split from #32489
ACKs for top commit:
Sjors:
ACK 47237cd1938058b29fdec242c3a37611e255fda0
w0xlt:
ACK 47237cd193
rkrux:
ACK 47237cd1938058b29fdec242c3a37611e255fda0
Tree-SHA512: 97c7f85b858efe5ced9b8aafb6cd7c1a547de6f8013b82bfc75bc567cf73c9db5e168e3980355756541305520022fd776b8d4d240d3fb34ed86c27d2acaf4863
9eb2c82e7c911a066781d67e6846cf6bbbaba6e9 walletdb: Remove unused upgraded_txs (Ava Chow)
c66803370988f9806c0ded24c404edb58f60498f wallet: Remove unused fTimeReceivedIsTxTime (Ava Chow)
Pull request description:
`CWalletTx::fTimeReceivedIsTxTime` is no longer used and can be removed. This additionally allows the removal of the `upgraded_txs` loop in `LoadWallet`.
ACKs for top commit:
maflcko:
lgtm ACK 9eb2c82e7c911a066781d67e6846cf6bbbaba6e9
Eunovo:
ACK 9eb2c82e7c
davidgumberg:
ACK 9eb2c82e7c
PeterWrighten:
ACK 9eb2c82e7c
rkrux:
ACK 9eb2c82e7c911a066781d67e6846cf6bbbaba6e9
w0xlt:
ACK 9eb2c82e7c
Tree-SHA512: 05cf3a50f0d8ab6ef423ad1113c5ce6f45bfdc90e2c0dcf61c2dceced2465502e574b4b5b0091fcbb4bdd2182f8d69224f1e5516c7c505de07102b84a5f40e9c
272cd09b796a36596b325277bb43cb47b19c8e12 log: Use warning level while scanning wallet dir (MarcoFalke)
17776443675ddf804f92042883ad36ed040438c3 qa, wallet: Verify warning when failing to scan (Hodlinator)
893e51ffeb0543e1c8d33e83b20c56f02d2b793c wallet: Correct dir iteration error handling (Hodlinator)
Pull request description:
Make wallet DB properly detect and report failure to scan wallet directory. Seems to have been broken since moving from Boost to `std::filesystem`.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
ACKs for top commit:
achow101:
ACK 272cd09b796a36596b325277bb43cb47b19c8e12
maflcko:
re-ACK 272cd09b796a36596b325277bb43cb47b19c8e12 🍽
rkrux:
tACK 272cd09b796a36596b325277bb43cb47b19c8e12
Tree-SHA512: 969afde2e37f885ed0c823dc36d2dbeaa0378639849c6a26f8ac67b4f1997eea95bbcae6d58aef5b716807210f37eb166c0cda7ba1d6caffd34249970833af3a
9dfc61d95f0082672a9b90528386e6bcd7014a78 test: detect no external signer connected (Sjors Provoost)
0a4ee93529d68a31f3ba6c7c6009954be47bbbd6 wallet: use PSBTError::EXTERNAL_SIGNER_NOT_FOUND (Sjors Provoost)
8ba2f9b7c8a6c6a91cc718d256354f7a73083b68 refactor: use util::Result for GetExternalSigner() (Sjors Provoost)
Pull request description:
When attempting to sign a transaction involving an external signer, if the device isn't connected we throw an `std::runtime_error`. This prevents the (mainly GUI) code that's actually supposed to handle this case from running.
This PR returns a `PSBTError::EXTERNAL_SIGNER_NOT_FOUND` instead of throwing.
The first commit is a refactor to have `GetExternalSigner()` return a `util::Result<ExternalSigner>` so the caller can decide how to handle the error. There are two other places where call `GetExternalSigner()` which this PR doesn't change (which I think is fine there).
Before:
After (the translation already exist):
Fixes#32426
Additionally use `LogWarning` instead of `std::cerr` for both a missing signer and failure to sign.
ACKs for top commit:
achow101:
ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78
brunoerg:
code review ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78
Tree-SHA512: 22515f4f0b4f50cb0ef532b729e247f11a68be9c90e384942d4277087b2e76806a1cdaa57fb51d5883dacf0a428e5279674aab37cce8c0d3d7de0f96346b8233
ce90f0c99fded22dd24f08757d6f48b5c6b52990 rpc, wallet, refactor: Remove non-descriptor errors (pablomartin4btc)
573bcd75d7b65ff02aaeea40d6f870a9c0bc7490 wallet, refactor: Remove unused SetupGeneration (pablomartin4btc)
5431f2dc2159f55e0fbe89d07deb97fe2a73fb43 wallet, refactor: Remove Legacy warnings and errors (pablomartin4btc)
Pull request description:
Remove dead code due to legacy wallet support removal.
These changes have no impact on functionality. They are transparent to the end user, as legacy wallets can't be created or loaded anymore, so these checks are no longer reached. The legacy-to-descriptor wallet migration flow is not affected either, as these removals are not part of its process.
ACKs for top commit:
achow101:
ACK ce90f0c99fded22dd24f08757d6f48b5c6b52990
rkrux:
utACK ce90f0c99fded22dd24f08757d6f48b5c6b52990
Tree-SHA512: 9229ad9dda9ff1dece73b5b15a20d69c6ab1ff2c75b2ec430ddbbaeb3467f6a850f53df527bcb4a8114ccbf1aa9c794462d71a8d516aed6f9a9da74edae16feb
0def84d407facd319b52826d013cad0d5fc8dbf5 test: Verify parent_desc in RPCs (Ava Chow)
2554cee988fb2ddf65428b354a238f1a4efc1aca test: Enable default wallet for wallet_descriptor.py (Ava Chow)
3fc9d9f241a44ab64774aa9ddc3ded4bb589ed5a wallet, rpc: Push the normalized parent descriptor (Ava Chow)
Pull request description:
Instead of prividing the descriptor string as stored in the db, use the normalized descriptor as is done for getaddressinfo's parent_desc field.
Split from #32489
ACKs for top commit:
Sjors:
re-utACK 0def84d407
rkrux:
ACK 0def84d407facd319b52826d013cad0d5fc8dbf5
w0xlt:
reACK 0def84d407
Tree-SHA512: 575c5b545d6f0aa7e135696b7a55c004e754fca4dd35dd9cf71b0b45b49a2e86e7b20570e768534d587005953bb893645379ec1ba4f98cfd26811f9c2f17de2d
130a922980778b293b22169d5e5649afde3ba33b wallet, interfaces: Use BERKELEY_RO in isEncrypted (Ava Chow)
Pull request description:
The GUI uses `WalletLoader::isEncrypted()` to detect whether a wallet file is encrypted so that it knows whether to prompt for a passphrase when migrating a legacy wallet. However, legacy wallets need to be opened with `options.require_format = BERKELEY_RO`. Since this wasn't being provided, following #28710, encrypted legacy wallets could not be migrated.
This fixes the issue by detecting when a wallet file is for a legacy wallet, and re-attempting with `options.require_format = BERKELEY_RO` in that case.
Depends on #32449 for `DatabaseStatus::FAILED_LEGACY_DISABLED`
ACKs for top commit:
davidgumberg:
Tested ACK 130a922980
furszy:
utACK 130a922980778b293b22169d5e5649afde3ba33b
pablomartin4btc:
tACK 130a922980778b293b22169d5e5649afde3ba33b
w0xlt:
Code review ACK 130a922980
rkrux:
utACK 130a922980778b293b22169d5e5649afde3ba33b
Tree-SHA512: aa70defc3b5f41635333a4d83c46ecdb5cd3cb129d590b4c0fe7a5f16e8aeaba1592f932ead242ed5f84524b146d87319154f4a1820bb34d9e80f63d24fc6b20
Seems to have been broken since conversion from Boost in #20744. The std::filesystem iteration aborts upon failure while Boost might have allowed skipping over faulty entries.
fa9ca13f35be0a023aeed78775ad66f95717b28b refactor: Sort includes of touched source files (MarcoFalke)
facb152697b8d7b75a9e6108f8896f774b06b35f scripted-diff: Bump copyright headers after include changes (MarcoFalke)
fae71d30f7227594e2f59499cf7f7f9420284e04 clang-tidy: Apply modernize-deprecated-headers (MarcoFalke)
Pull request description:
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).
The check is currently disabled for headers, to exclude subtree headers.
ACKs for top commit:
l0rinc:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
achow101:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
janb84:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
stickies-v:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
Tree-SHA512: 6639608308c598d612e24435aa519afe92d71b955874b87e527245291fb874b67f3ab95d3a0a5125c6adce5eb41c0d62f6ca488fbbfd60a94f2063d734173f4d
It is not possible to load a legacy/ non-descriptor wallet anymore
so no need to check for WALLET_FLAG_DESCRIPTORS in RPC calls, even when
passing -rpcwallet/ JSON `/wallet/<walletname>/` endpoint, that searches
for the wallets loaded already in the context.
SetupGeneration was supposed to be the function that all SPKMs used
to setup automatic generation, but it didn't work out that way and
ended up being legacy only. It should be deleted at this point.
This RPC lists all the descriptors present in the wallet, not only
the ones that were imported, but also the ones generated when a
new wallet is created.
It can be verified by creating a new wallet and calling the
`listdescriptors` RPC, which will contain 8 ranged descriptors that
are created for every new wallet.
Also, update the description to get rid of "descriptor-enabled"
because this is the only wallet type available now after removal of
legacy wallets.
Instead of throwing a runtime error, let the caller decide how to handle a missing signer.
GUI code was already in place to handle this, but it was unused until this commit.
Fixes#32426
Additionally use LogWarning instead of std::cerr.
f98e1aaf34e347088caa54403521e3b5cb55dd40 rpc: Note in fundrawtransaction doc, fee rate is for package (benthecarman)
Pull request description:
Accidentally made some transactions with a much higher fee rate than I wanted because I did not know this would do it for the package rather than the individual tx.
ACKs for top commit:
achow101:
ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
rkrux:
re-ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
danielabrozzoni:
ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
Tree-SHA512: 9f961de1200803ec4d1c6901fd606bb6cf707ffd03942d9dc0d4b6554c827075f99d693b93e892f728679d67e63e12c71da4426dab091b3311d1605bc37251a2
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
86e1111239cdb39dd32cfb5178653c608fa30515 test: verify node skips loading legacy wallets during startup (furszy)
9f94de5bb54ff683bd4d3a7723617b34a4706bb6 wallet: init, don't error out when loading legacy wallets (furszy)
Pull request description:
Instead of failing during initialization and shutting down the app when encountering a legacy wallet, skip loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli users will be able to call the migratewallet RPC directly after init.
ACKs for top commit:
achow101:
ACK 86e1111239cdb39dd32cfb5178653c608fa30515
w0xlt:
ACK 86e1111239
Tree-SHA512: 85d594a503ee7a833a23754b71b6ba4869ca34ed802c9ac0cd7b2fa56978f5fcad84ee4bd3acdcc61cf8e7f08f0789336febc5d76beae1eebf7bd51462512b78
ad9a13fc424e9deb262e2b1d54bcdc7370263ea0 walletdb: Log additional exception error messages for corrupted wallets (Ava Chow)
Pull request description:
Many exceptions thrown for corruption are `std::runtime_error`; we should catch those and log the message to help with debugging.
Split from #32489
ACKs for top commit:
davidgumberg:
ACK ad9a13fc42
furszy:
ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
rkrux:
ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
Sjors:
utACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
Tree-SHA512: 107b938d67346804733ea27c44ed38822db0e020e5b1ac889ee35280d812ec56dcc9af7b3eab7a521d72cdd9cb4a8d6d35f3a3dfbcb2a6fd170a981f34fbdfc2
Although WalletBatch::LoadWallet performs the descriptor cache upgrade,
because new wallets do not have the descriptor flag set yet, the upgrade
does not run and set the flag.
Since new wallets will always being using the upgraded cache, there's no
reason to wait to set the flag, so set it when the wallet flags are
being initialized for new wallets.
This is the RPC example counterpart to commit
86de8c1668005304b2c630ca2ad4a8ca8e348e90 (PR #32544).
Since the recent legacy wallet removal this parameter *must* be
true, so providing it in the examples doesn't contain valuable
information anymore and it seems best to remove them.
Now that we only ever operate on descriptor wallets, mentioning
that a faster rescan is only available for them is redundant and
can be removed.
These texts were originally introduced in commit
ca48a4694f73e5be8f971ae482ebc2cce4caef44 (PR #25957).
This `getwalletinfo()` result field was only ever returned for
legacy wallets and is hence not relevant anymore, so we can
delete it and the corresponding CWallet/ScriptPubKeyMan code
behind it.
785e1407b0a39fef81a7b25554aab88d4cecd66b wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)
Pull request description:
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.
ACKs for top commit:
Sjors:
utACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
ryanofsky:
Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
furszy:
Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
e8661aac752eb08fee318eb8f56e599578d78f9f wallet: drop watch-only things from interface (Sjors Provoost)
e99188e7daa19dcb14f387931949edf581ae5f37 qt: drop unused watch-only functionality (Sjors Provoost)
Pull request description:
The watch-only functionality in the GUI was only used for legacy wallets. Watch-only descriptor wallets do not use this.
The only visible changes of this PR should be:
- dropped "Spendable:" label from the overview tab
- column width cache is reset
This PR also removes some unused variables from the interface.
ACKs for top commit:
davidgumberg:
Review ACK e8661aac75.
hebasto:
ACK e8661aac752eb08fee318eb8f56e599578d78f9f, I have reviewed the code and it looks OK. The `src/qt/forms/overviewpage.ui` form was reviewed in Qt Designer.
Tree-SHA512: d7edb0f167e0b934075398a76eddca69890bb36848a918c932b1c2cea85ee87285e876cbfdf1f6dec7adf26b9f405fb558c70bec0c84585c0a9df33c2af78393
ee045b61efc1479c1866b786661ef39a863677d0 rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c3729d4e054ac4260b344a75ad4b7239b3 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06709212934c521c513bb2e1a521a31f psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f80e998f7402433572b695f589f7f42 psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337ad75390a1f8810d6715f3634ed07e98 wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49b2e709a2b00af92ca76e9f30e47aec psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a11825694856a2643e9600fa537182fbb597c107 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4ada5b64c8113450ed736a2581c97518 wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923b10f0690de9dcd34f17fbb8d6de5eb psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd815325713d64b9daa61c2f93061d27bd47d tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d6123e0056656e406cd9f35aac6f71df4b psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
rkrux:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
fjahr:
Code review ACK ee045b61efc1479c1866b786661ef39a863677d0
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
97d383af6d54b463da64f45680a146d7e93eb146 Test updating non-ranged descriptor with [0,0] range succeeds (Novo)
2ae1788dd46a71cb47ec25e149ea11fa8689c64f Skip range verification for non-ranged desc (Novo)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/31728
This PR updates the `DescriptorScriptPubKeyMan` to skip range checks for non-ranged descriptors, which previously caused errors when updating a non-ranged descriptor with the range [0,0]
#### Testing
A unit test was added to test the new behaviour
ACKs for top commit:
achow101:
ACK 97d383af6d54b463da64f45680a146d7e93eb146
rkrux:
ACK 97d383a
Tree-SHA512: 6dbd058376d9e57d26477d9d6d89646e80a32e3ffcc9f4e30eeda273575d12583ce520cc0032cc67c12ea0b3ad344fbd3945d9fc5e389b6a6bce1ea7ad5d6e59
6f7052a7b96f058568af9aed2f014ae7a25e0f68 threading: semaphore: move CountingSemaphoreGrant to its own header (Cory Fields)
fd1546989293b110ad8d86d71f362a11dab3611c threading: semaphore: remove temporary convenience types (Cory Fields)
1f89e2a49a2170a57b14d993f181f29233b7d250 scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones (Cory Fields)
f21365c4fc7f6f45194f5b725192f0054e2daf13 threading: replace CountingSemaphore with std::counting_semaphore (Cory Fields)
1acacfbad780f95d1596010ba446dd9ea268fa10 threading: make CountingSemaphore/CountingSemaphoreGrant template types (Cory Fields)
e6ce5f9e78741ef7f88a8ad237f4b772da921dc3 scripted-diff: rename CSemaphore and CSemaphoreGrant (Cory Fields)
793166d3810ef3c08cc55c16a17d6d77ae6fabb5 wallet: change the write semaphore to a BinarySemaphore (Cory Fields)
6790ad27f1570926cef81ef097edaa8b8e70b270 scripted-diff: rename CSemaphoreGrant and CSemaphore for net (Cory Fields)
d870bc94519a68a861bb0ceca19f96c6ba22fbd7 threading: add temporary semaphore aliases (Cory Fields)
7b816c4e00e286a6dcdf0d9e09c710e1d745a0db threading: rename CSemaphore methods to match std::semaphore (Cory Fields)
Pull request description:
This is relatively simple, but done in a bunch of commits to enable scripted diffs.
I wanted to add a semaphore in a branch I've been working on, but it was unclear if I should use `std::counting_semaphore` or stick with our old `CSemaphore`. I couldn't decide, so I just decided to remove all doubt and get rid of ours :)
This replaces our old `CSemaphore` with `std::counting_semaphore` everywhere we used it. `CSemaphoreGrant` is still there as an RAII wrapper, but is now called `CountingSemaphoreGrant` and `BinarySemaphoreGrant` to match. Those have been moved out of `sync.h` to their own file.
ACKs for top commit:
purpleKarrot:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
achow101:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
TheCharlatan:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
hebasto:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68, I have reviewed the code and it looks OK.
Tree-SHA512: 5975d13aa21739174e3a22c544620ae3f36345f172b51612346d3b7baf0a07c39ef6fd54f647c87878c21a67951b347a5d4a5f90e897f3f6c0db360a3779d0df
faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40 doc: Remove ParseInt mentions in documentation (MarcoFalke)
33332829333b589420f8038541d04ec6970f051d refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c36cb0accf87123ca4eb98f6219d55fb5e bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face2519fac9e840d52f0334d9079e664be7eb28 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf0b993c879ee8c516baa36339ff7b72406 bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a558ab15b5d8eeea5dd4c9c0d76350051b bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5fe38b81f1af6b343661b65e16b0de7c60 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac03788243847b799a3feaac56bc918fba9 rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb499dec79258b1857b404d72f93650503f4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c69192fcb48a9e04d52eeb07fff15655d0 test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123afa0ef752e8645bf695d121da66d8f1382b Reject + sign when checking -ipcfd (MarcoFalke)
fa479857ed234d54df31d33b60de14c6ffab3d6f Reject + sign in SplitHostPort (MarcoFalke)
fab4c2967d554ddbc15f732cea6cd190c547d89f net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652e68fc07fb6c9f3d9e34dc11d35f0cc1e1 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c45577dfbae67535e4965b5660288557d3631 cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa980413257e2004a8d48a8be66c6d67f90b76ad refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7fc24212d85cdc7f52b317906b37a1a120 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)
Pull request description:
The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:
* Useless, because the `+` sign was already rejected.
* Erroneous and inconsistent, when third party parsers reject it. (C.f. https://github.com/bitcoin/bitcoin/pull/32365)
* Confusing, because the `+` sign is neither documented, nor can it be assumed to be present.
Fix all issues by removing the legacy int parsing.
ACKs for top commit:
stickies-v:
re-ACK faf55fc80b
brunoerg:
code review ACK faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40
Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
4b2cd0b41ff4800c8801f2c44883eaec60a035fa test: check that creating a wallet does not log version info (Ava Chow)
39a483c8e9dcfe8ec243fa72269e1df9e75059ab test: Check that the correct versions are logged on wallet load (Ava Chow)
359ecd3704993422eb53e3da2a7d0bea2f575ab0 walletdb: Log the wallet version after it has been read from disk (Ava Chow)
Pull request description:
The wallet's version (in the minversion record) needs to be logged only after we have read it from disk. Otherwise, we always log the lowest version number of 10500 which is incorrect. Furthermore, it doesn't make sense to log the last client version number if the record didn't exist. This is a regression caused by #26021.
The wallet file version logging is moved inside of `LoadMinVersion` so that it is logged after the record is read. It will also log unconditionally if a version is read so that the version number is reported even when there is an error. The last client logging is split into its own log line that will only occur if a last client record is read. The only situation where we expect no version numbers to be logged is when a wallet is being created.
A test is added in the second commit to check that the version number is correctly logged on loading. This commit can be cherrypicked to master to verify that it fails there. The last commit adds an additional check that creating a new wallet does not log any version info at all.
ACKs for top commit:
laanwj:
Code review ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
janb84:
ACK 4b2cd0b41f
furszy:
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
rkrux:
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
Tree-SHA512: b30c76f414d87be6c14b42d2d3c8794a91a7e8601501f4c24641d51ff2b5c5144776563baf41ca1c38415844740b760b19a3e5791f78013b39984dfedd3b1de7
7193245cd66791216d4e586ca09302b26d4b7528 doc: remove For ... comments (fanquake)
1b9cdc933f6c11cb8593b5ad9ae3f4eb2c726859 net: drop win32 ifdef (fanquake)
19ba499b1f3884ea69c5f833f3eb797f90372aa5 init: cerrno is used on all platforms (fanquake)
Pull request description:
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs, if changed.
They are also wrong, i.e `DEFAULT_SCRIPTCHECK_THREADS` is not in
`validation.h`.
ACKs for top commit:
stickies-v:
re-ACK 7193245cd66791216d4e586ca09302b26d4b7528
fjahr:
ACK 7193245cd66791216d4e586ca09302b26d4b7528
willcl-ark:
reACK 7193245cd66791216d4e586ca09302b26d4b7528
Tree-SHA512: 6b5f83cd1df699356e1cbb78949f8d456b13ce288f0064138118cfb45b4c77e2d1945babe91598dffe9823ab07dfae36f4c3b61c586cf98baf16890bdf322b08
32023 changed AddWalletDescriptor to return util::Error, but did not
change all of the failure cases to do so. This may result in some
callers continuing when there was actually an error. Unify all of the
failure cases to use util::Error so that all callers handle
AddWalletDescriptor errors in the same way.
The encapsulated return type is changed from ScriptPubKeyMan* to
std::reference_wrapper<DescriptorScriptPubKeyMan>. This avoids having a
value that can be interpreted as a bool, and also removes the need to
constantly dynamic_cast the returned value. The only kind of
ScriptPubKeyMan that can come out of AddWalletDescriptor is a
DescriptorScriptPubKeyMan anyways.
30a94b1ab9ae850d55cb9eb606a06890437bc75e test, wallet: Remove concurrent writes test (Ava Chow)
b44b7c03fef01e0b5db704e50762b3d16b3da69e wallet: Write best block record on unload (Ava Chow)
876a2585a8b69e12ac171a0d9ff5aab864067c42 wallet: Remove unnecessary database Close step on shutdown (Ava Chow)
98a1a5275c8c395fe47ff7f10109d75b06bc391d wallet: Remove chainStateFlushed (Ava Chow)
7fd3e1cf0c88553e0722048ce488f265883558e7 wallet, bench: Write a bestblock record in WalletMigration (Ava Chow)
6d3a8b195a826448c021dd189255ca41ba70cc5a wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed (Ava Chow)
7bacabb204b6c34f9545f0b37e2c66296ad2c0de wallet: Update best block record after block dis/connect (Ava Chow)
Pull request description:
Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484
Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.
This PR changes those last block fields to be updated whenever the wallet makes a change to the db for new transaction state found in new blocks. Whenever a block is received that contains a transaction relevant to the wallet, the last block locator will now be written to disk. Furthermore, every block disconnection will now write an updated locator.
To ensure that the locator is relatively recent and loading rescans are fairly quick in the event of unplanned shutdown, it is also now written every 144 blocks (~1 day). Additionally it is now written when the wallet is unloaded so that it is accurate when the wallet is loaded again.
Lastly, the `chainstateFlushed` notification in the wallet is changed to be a no-op. The best block locator record is no longer written when `chainstateFlushed` is received from the node since it should already be mostly up to date.
ACKs for top commit:
rkrux:
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
mzumsande:
Code Review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
ryanofsky:
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.
Tree-SHA512: 46117541f8aaf13dde57430e813b4bbbd5e146e2632769675803c8e65a82f149a7cc6026489a127d32684b90124bd2b7c28216dbcfa6a47447300e8f3814e029
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
