| Age | Commit message (Collapse) | Author |
|---|
|
Updates from Chain Interface
1b41d45d462d856a9d0b44ae0039bbb2cd78407c wallet: bugfix: ensure atomicity in settings updates (ismaelsadeeq)
Pull request description:
This PR fixes #30620.
As outlined in the issue, creating two wallets with `load_on_startup=true` simultaneously results in only one wallet being added to the startup file.
The current issue arises because the wallet settings update process involves:
1. Obtaining the settings value while acquiring the settings lock.
2. Modifying the settings value.
3. Overwriting the settings value while acquiring the settings lock again.
This sequence is not thread-safe. Different threads could modify the same base value simultaneously, overwriting data from other workers without realizing it.
The PR attempts to fix this by modifying the chain interface's `updateRwSetting` method to accept a function that will be called with the settings reference. This function will either update or delete the setting and return an enum indicating whether the settings need to be overwritten in this or not.
Additionally, this PR introduces two new methods to the chain interface:
- `overwriteRwSetting`: This method replaces the setting with a new value.
Used in `VerifyWallets`
- `deleteRwSettings`: This method completely erases a specified setting.
This method is currently used only in `overwriteRwSetting`.
These changes ensure that updates are race-free across all clients.
ACKs for top commit:
achow101:
ACK 1b41d45d462d856a9d0b44ae0039bbb2cd78407c
furszy:
self-code-ACK https://github.com/bitcoin/bitcoin/commit/1b41d45d462d856a9d0b44ae0039bbb2cd78407c
Tree-SHA512: 50cda612b782aeb5e03e2cf63cc44779a013de1c535b883b57af4de22f24b0de80b4edecbcda235413baec0a12bdf0e5750fb6731c9e67d32e742d8c63f08c13
|
|
- Settings updates were not thread-safe, as they were executed in
three separate steps:
1) Obtain settings value while acquiring the settings lock.
2) Modify settings value.
3) Overwrite settings value while acquiring the settings lock.
This approach allowed concurrent threads to modify the same base value
simultaneously, leading to data loss. When this occurred, the final
settings state would only reflect the changes from the last thread
that completed the operation, overwriting updates from other threads.
Fix this by making the settings update operation atomic.
- Add test coverage for this behavior.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
|
|
|
|
6ed424f2db609f9f39ec1d1da2077c7616f3a0c2 wallet: fix, detect blank legacy wallets in IsLegacy (furszy)
Pull request description:
Blank legacy wallets do not have active SPKM. They can only be
detected by checking the descriptors' flag or the db format.
This enables the migration of blank legacy wallets in the GUI.
To test this:
1) Create a blank legacy wallet.
2) Try to migrate it using the GUI's toolbar "Migrate Wallet" button.
-> In master: The button will be disabled because `CWallet::IsLegacy()` returns false for blank legacy wallet.
-> In this PR: the button will be enabled, allowing the migration of legacy wallets.
ACKs for top commit:
achow101:
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
tdb3:
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
glozow:
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
Tree-SHA512: c06c4c4c2e546ccb033287b9aa3aee4ca36b47aeb2fac6fbed5de774b65caef9c818fc8dfdaac6ce78839b2d5d642a5632a5b44c5e889ea169ced80ed50501a7
|
|
f550a8e035b4603787273ea250f403f6f0be453f Rename ReleaseWallet to FlushAndDeleteWallet (furszy)
64e736d79efc7201768244fc297084f70c0bebc1 wallet: WaitForDeleteWallet, do not expect thread safety (Ryan Ofsky)
8872b4a6ca91a83bf8d5a118fb808c043b9e879d wallet: rename UnloadWallet to WaitForDeleteWallet (furszy)
5d15485aafefdc759ba97e039bb1b9ccac267358 wallet: unload, notify GUI as soon as possible (furszy)
Pull request description:
Coming from #29073.
Applied ryanofsky suggested changes on https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242 with few modifications coming from https://github.com/bitcoin/bitcoin/pull/18338#issuecomment-605060348.
The only point I did not tackle from https://github.com/bitcoin/bitcoin/pull/18338#issuecomment-605060348 is:
> * Move log print and flush out of ReleaseWallet into CWallet destructor
Because it would mean every `CWallet` object would flush data to disk during destruction. Which is not necessary for wallet tool utilities and unit tests.
ACKs for top commit:
achow101:
ACK f550a8e035b4603787273ea250f403f6f0be453f
ryanofsky:
Code review ACK f550a8e035b4603787273ea250f403f6f0be453f. Just a simple rename since last review
ismaelsadeeq:
Re-ACK f550a8e035b4603787273ea250f403f6f0be453f
Tree-SHA512: e2eb69bf36883c514f601f4838ae6a41113996b9559abf8dc2b46e16bbcdad401195ac0f2b9d1fb55a10e78bb8ea9953788a168c80474e3f101350d208cb3bd2
|
|
To better describe the function's behavior.
And add wallet name to logprint.
|
|
Multiple threads could try to delete the wallet at the same time.
|
|
And update function's documentation.
|
|
Releases wallet shared pointers prior to doing the
final settings update and prevent GUI races trying
to access a wallet that is no longer loaded.
|
|
8f2522d242961ceb9e79672aa43e856863a1a6dd gui: Use menu for wallet migration (Ava Chow)
d56a450bf5172e2c3f4b9a2786e71268019e1277 gui: Use wallet name for wallet migration rather than WalletModel (Ava Chow)
c3918583dd5fcd9001136da2192e02e092128901 gui: don't remove wallet manually before migration (furszy)
bfba63880fbb1108b73540faeb0620ba24b8cdd0 gui: Consolidate wallet display name to GUIUtil function (Ava Chow)
28fc562f2692af4f37f918d4ae31c4d115e03aee wallet, interfaces: Include database format in listWalletDir (Ava Chow)
Pull request description:
Currently the Migrate Wallet menu item can only be used to migrate the currently loaded wallet. This is not suitable for the future when legacy wallets can no longer be loaded at all, but should still be able to be migrated. This PR changes that menu item into a menu list like Open Wallet and lets users migrate any legacy wallet in their wallet directory regardless of the wallets loaded.
One issue I ran into was dealing with encrypted wallets. Ideally, we would detect whether a wallet is encrypted, and prompt the user for their passphrase at that time. However, that's actually difficult to do in the GUI since migration will unload the wallet if it was already loaded, and reload it without connecting it to any signals or interfaces. Only then can it detect whether a wallet is encrypted, but then there is no `WalletModel` or even an `interfaces::Wallet` that the GUI could use to unlock it via a callback.
To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted. If the user enters the wrong passphrase or clicked the other button that does not prompt for the passphrase, migration will fail with a message indicating that the passphrase was incorrect.
ACKs for top commit:
hebasto:
ACK 8f2522d242961ceb9e79672aa43e856863a1a6dd.
furszy:
ACK 8f2522d
Tree-SHA512: a0e3b70dbfcacb89617956510ebcea94cad8617a987c68fe39fa16ac1721190b7cf7afc156c39b9032920cfb67b5d4ca28791681f5021d92d16acc691387afa1
|
|
To prepare for migrating wallets that are not loaded, when migration
occurs in the GUI, it should not rely on a WalletModel existing.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
|
|
|
|
Blank legacy wallets do not have active SPKM. They can
only be detected by checking the descriptors' flag or
the db format.
This enables the migration of blank legacy wallets in
the GUI.
|
|
401cc4ec70d67ba2aa0e078d2fab214e1c40742c fuzz: improve scriptpubkeyman target (brunoerg)
Pull request description:
Fixes #30541
This PR aims to improve `scriptpubkeyman` target to avoid timeouts. The input provided in #30541 takes too much time to run because it basically calls only `MarkUnusedAddresses` (300 times * number of spks). The following changes were made to improve it:
- Reduce keypool size.
- When calling `MarkUnusedAddresses`, do it with one of the spks per iteration.
- Remove the specific `AddDescriptorKey` call since it is already covered with `AddWalletDescriptor`.
- Limit number of iterations to a reasonable value.
ACKs for top commit:
maflcko:
lgtm ACK 401cc4ec70d67ba2aa0e078d2fab214e1c40742c
achow101:
ACK 401cc4ec70d67ba2aa0e078d2fab214e1c40742c
Tree-SHA512: 941812bc6d991dd03675a2974ce1b839494ca7f6e6d8a22c689d4bf4fed2dac5491246998f19cb15dbff516fdd8eeda27e7628c3206d45f57dc292bc05624a5c
|
|
default wallets and generated backup files
6b2dcba07670f04f32c0dc3a2c86fd805c85f12d wallet: List sqlite wallets with empty string name (Ava Chow)
3ddbdd1815c676a88345b3b0e55a551d2a569e28 wallet: Ignore .bak files when listing wallet files (Ava Chow)
Pull request description:
When the default wallet is migrated, we do not rename the wallet so we end up having a descriptor wallet with the empty string as its name and the wallet.dat file in the root of the walletdir. This is supposed to be an unsupported configuration and there is no other way to achieve this (other than file copying), but the wallet loading code does not disallow loading such wallets. However `listwalletdir` does not currently list the default wallet if it is sqlite. This is confusing to users, so change `listwalletdir` to include these wallets.
Additionally, the migration of the default wallet, and of any plain wallet files in the walletdir, produces a backup file in the walletdir itself. Since these backups are a BDB file, `listwalletdir` will detect them as being another wallet that we could open, but this is erroneous and could lead to confusion and potentially funds loss if both the backup and the migrated wallet are in use simultaneously. To reduce the likelihood of this issue, don't list these wallets in `listwalletdir`.
***
Possibly we could have more stringent checks on loading to resolve these issues, but I'm concerned that that will just confuse users and gratuitously break things that already worked.
Since the original intent was to disallow default wallets for sqlite/descriptors, a possible alternative would be to prevent people from loading such wallets and change migration to rename those wallets. However, given that this behavior with migrating default wallets has existed since default wallet migration was fixed, I think that making such a change would be confusing and break things for no good reason. Although perhaps we should still do the renaming.
For the backups, we could also change loading to refuse to load any wallet named with `.bak` (or `.legacy.bak`) as such wallets can still be loaded by giving the path to them directly, which some users may do to "restore" the backup. However restricting what can be loaded based on filename seems a little heavyhanded. It wouldn't be funds loss though since the correct way to restore the backup is with `restorewallet`.
ACKs for top commit:
fjahr:
Code review ACK 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d
furszy:
Code ACK 6b2dcba076
glozow:
ACK 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d
Tree-SHA512: 0b033f6ed55830f8a054afea3fb2cf1fa82a94040053ebfaf123bda36c99f45d3f01a2aec4ed02fed9c61bb3d320b047ed892d7f6644b5a356a7bc5974b10cff
|
|
fa5755b0a8536b844fdccfecf386c1baab24f1c9 doc: rpc: Use "output script" consistently (2/2) (MarcoFalke)
Pull request description:
Small follow-up to https://github.com/bitcoin/bitcoin/pull/30408 to fixup the RPCs that were forgotten.
ACKs for top commit:
theStack:
lgtm ACK fa5755b0a8536b844fdccfecf386c1baab24f1c9
Tree-SHA512: f1fc0aabb59017da216d6fe0f08a2274336d04db332ad6ce3d9608cd6f03667be1c76423f24a489ac8e7d536011a129dca752ab64b4621b7bc1d4d53f68602e4
|
|
Although it is not explicitly possible to create a default wallet with
descriptors, it is possible to migrate a default wallet and have it end
up being a default wallet with descriptors. These wallets should be
listed by ListDatabases so that it appears in wallet directory listings
to avoid user confusion.
|
|
Migration creates backup files in the wallet directory with .bak as the
extension. This pollutes the output of listwalletdir with backup files
that most users should not need to care about.
|
|
fa2f26960ee084971ab09959b213a9b8104482e5 [rpc, fees]: add more detail on the fee estimation modes (ismaelsadeeq)
6e7e620864cc7a2f3c3b576588afe4d44dc394ec [doc]: add `30275` release notes (ismaelsadeeq)
Pull request description:
This PR:
1. Adds release notes for #30275
2. Describe fee estimation modes in RPC help texts
ACKs for top commit:
achow101:
ACK fa2f26960ee084971ab09959b213a9b8104482e5
glozow:
ACK fa2f26960ee084971ab09959b213a9b8104482e5
willcl-ark:
ACK fa2f26960ee
tdb3:
re ACK fa2f26960ee084971ab09959b213a9b8104482e5
Tree-SHA512: b8ea000b599297b954dc770137c29b47153e68644c58550a73e34b74ecb8b65e78417875481efdfdf6aab0018a9cd1d90d8baa5a015e70aca0975f6e1dc9598c
|
|
fa18fc705084f3620be566d8c6639b29117ccf68 log: Remove NOLINT(bitcoin-unterminated-logprintf) (MarcoFalke)
Pull request description:
`NOLINT(bitcoin-unterminated-logprintf)` is used to document a missing trailing `\n` char in the format string. This has many issues:
* It is just documentation, assuming that a trailing `\n` ends up in the formatted string. It is not enforced at compile-time, so it is brittle.
* If the newline was truly missing and `NOLINT(bitcoin-unterminated-logprintf)` were used to document a "continued" line, the log stream would be racy/corrupt, because any other thread may inject a log message in the meantime.
* If the newline was accidentally missing, nothing is there to correct the mistake.
* The intention of all code is to always end a log line with a new line. However, historic code exists to deal with the case where the new line was missing (`m_started_new_line`). This is problematic, because the presumed dead code has to be maintained (https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682963306).
Fix almost all issues by removing the `NOLINT(bitcoin-unterminated-logprintf)`, ensuring that a new line is always present.
A follow-up will remove the dead logging code.
ACKs for top commit:
TheCharlatan:
ACK fa18fc705084f3620be566d8c6639b29117ccf68
ryanofsky:
Code review ACK fa18fc705084f3620be566d8c6639b29117ccf68
Tree-SHA512: bf8a83723cca84e21187658edc19612da79c34f7ef2e1f6e9353e7ba70e4ecc0a878a2ae32290045fb90cba9a44451e35341a36ef2ec1169d13592393aa4a8ca
|
|
bfd3c29e4f7942b49986ce0efa08481bae190b7e fuzz: fix timeout in crypter target (brunoerg)
Pull request description:
Fixes #30503
- Move SetKeyFromPassphrase to out of LIMITED_WHILE
- Remove `SetKey` calls since it is already called internally by other functions.
- Reduce number of iterations (100 is enough, no need for 10,000).
ACKs for top commit:
maflcko:
review ACK bfd3c29e4f7942b49986ce0efa08481bae190b7e 📆
dergoegge:
utACK bfd3c29e4f7942b49986ce0efa08481bae190b7e
Tree-SHA512: 275ab7d07a20bfd07279a23613678993c10c166f40cdc900213b9f4d5afb107462d5f88518a0f4ce2a52f3b7950ff2c01cf74292042f16996909fcb96f827d3e
|
|
- Add description that indicates the fee estimation modes behaviour.
- This description will be returned in the RPC's help texts.
|
|
Move `SetKeyFromPassphrase` to out of LIMITED_WHILE,
remove `SetKey` calls since it is already called
internally by other functions and reduce the number
of iterations.
|
|
The goal of this improvement is to reduce
TopUp calls which can lead to timeouts.
|
|
These outputs are called anchors, and allow
key-less anchor spends which are vsize-minimized
versus keyed anchors which require larger outputs
when creating and inputs when spending.
|
|
|
|
|
|
|
|
funding options (take 2)
734076c6de1781f957c8bc3bf7ed6951920cfcf6 [wallet, rpc]: add `max_tx_weight` to tx funding options (ismaelsadeeq)
b6fc5043c16c2467a2a6768a6ca9b18035fc400f [wallet]: update the data type of `change_output_size`, `change_spend_size` and `tx_noinputs_size` to `int` (ismaelsadeeq)
baab0d2d43049a71dc90176bc4d72062f7b2ce19 [doc]: update reason for deducting change output weight (ismaelsadeeq)
7f61d31a5cec8fc61328bee43f90d3f1dcb0a035 [refactor]: update coin selection algorithms input parameter `max_weight` name (ismaelsadeeq)
Pull request description:
This PR taken over from #29264
The PR added an option `max_tx_weight` to transaction funding RPC's that ensures the resulting transaction weight does not exceed the specified `max_tx_weight` limit.
If `max_tx_weight` is not given `MAX_STANDARD_TX_WEIGHT` is used as the max threshold.
This PR addressed outstanding review comments in #29264
For more context and rationale behind this PR see https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs
ACKs for top commit:
achow101:
ACK 734076c6de1781f957c8bc3bf7ed6951920cfcf6
furszy:
utACK 734076c6de1781f957c8bc3bf7ed6951920cfcf6
rkrux:
reACK [734076c](https://github.com/bitcoin/bitcoin/pull/29523/commits/734076c6de1781f957c8bc3bf7ed6951920cfcf6)
Tree-SHA512: 013501aa443d239ee2ac01bccfc5296490c27b4edebe5cfca6b96c842375e895e5cfeb5424e82e359be581460f8be92095855763a62779a18ccd5bdfdd7ddce7
|
|
fa6390df205513319f28e35e3e17c40ecaa7d731 doc: getaddressinfo[isscript] is optional (MarcoFalke)
Pull request description:
`isscript` is unknown for unknown witness versions, so it should be marked optional in the docs
Fixes https://github.com/bitcoin/bitcoin/issues/30456
ACKs for top commit:
stickies-v:
ACK fa6390df205513319f28e35e3e17c40ecaa7d731
tdb3:
ACK fa6390df205513319f28e35e3e17c40ecaa7d731
Tree-SHA512: f728f18e0871923225e0bf29594f8095997456cf55409f42087b5f70f95bef10f984323b48d2b484b6705f23b04e9e8a3fe42446830638fdd70453c18fd7f189
|
|
|
|
returning `complete=true`
7e36dca657c66bc70b04d5b850e5a335aecfb902 test: add test for modififed walletprocesspsbt calls (willcl-ark)
39cea21ec51b9838669c38fefa14f25c36ae7096 wallet: fix FillPSBT errantly showing as complete (willcl-ark)
Pull request description:
Fixes: #30077
Fix cases of calls to `FillPSBT` returning `complete=true` when it's not
the case.
This can happen when some inputs have been signed but the transaction is
subsequently modified, e.g. in the context of PayJoins.
Also fixes a related bug where a finalized hex string is attempted to be
added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort.
ACKs for top commit:
achow101:
ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902
ismaelsadeeq:
Tested ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902
pinheadmz:
re-ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902
Tree-SHA512: e35d19789899c543866d86d513506494d672e4bed9aa36a995dbec4e72f0a8ec5536b57c4a940a18002ae4a8efd0b007c77ba64e57cd52af98e4ac0e7bf650d6
|
|
4383dc90bac1b5def73352fe222f99807d8ca4dd fuzz: fix key size in crypter target (brunoerg)
Pull request description:
Fixes #30251
This PR:
1. Limits `cipher_text_ed` and `random_string` (`SecureString`) size.
2. Replace `ConsumeRandomLengthByteVector` for keys to `ConsumeFixedLengthByteVector` with `WALLET_CRYPTO_KEY_SIZE`.
3. Replace `ConsumeRandomLengthByteVector` for `chSalt` to `ConsumeFixedLengthByteVector` with `WALLET_CRYPTO_SALT_SIZE`.
ACKs for top commit:
marcofleon:
Tested ACK 4383dc90bac1b5def73352fe222f99807d8ca4dd. I ran this:
dergoegge:
utACK 4383dc90bac1b5def73352fe222f99807d8ca4dd
Tree-SHA512: 6f09cca0b4627f49152b685ac03659c01004f2131c6aada7654606ea01f6619b1611b1d17624d2cddce277c1afdddda5f656d99f6ca8f72a22f5c0541762c964
|
|
3333bae9b2a6c1ee2314d33361c93944c12001f9 tidy: modernize-use-equals-default (MarcoFalke)
Pull request description:
Prior to C++20, `modernize-use-equals-default` could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that aggregate initialization would be enabled where the author did not intend to enable it.
With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)
So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html
ACKs for top commit:
stickies-v:
ACK 3333bae9b2a6c1ee2314d33361c93944c12001f9
Tree-SHA512: ab42ff01be7ca7e7d8b4c6a485e68426f59627d83dd827cf292304829562348dc17a52ee009f5f6f3c1c2081d7166ffac4baef23197ebeba8de7767c6ddfe255
|
|
wallets without requiring BDB
8ce3739edbcf6437bf2695087e0ebe8c633df19b test: verify wallet is still active post-migration failure (furszy)
771bc60f134c002a027e799639f0fed59ae8123d wallet: Use LegacyDataSPKM when loading (Ava Chow)
61d872f1b3d0dbfd0de4ff9b7deff40eeffa6a14 wallet: Move MigrateToDescriptor and DeleteRecords to LegacyDataSPKM (Ava Chow)
b231f4d556876ae70305e8710e31d53525ded8ae wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM (Ava Chow)
7461d0c006c92ede2f2595b79a5509eaf3509fb7 wallet: Move LegacySPKM data storage and handling to LegacyDataSPKM (Ava Chow)
517e204bacd9dcea6612aae57d5a2813b89cd01d Change MigrateLegacyToDescriptor to reopen wallet as BERKELEY_RO (Ava Chow)
Pull request description:
#26606 introduced `BerkeleyRODatabase` which is an independent parser for BDB files. This PR uses this in legacy wallet migration so that migration will continue to work once the legacy wallet and BDB are removed. `LegacyDataSPKM` is introduced to have the minimum data and functions necessary for a legacy wallet to be loaded for migration.
ACKs for top commit:
cbergqvist:
ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b
theStack:
Code-review ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b
furszy:
Code review ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b
Tree-SHA512: dccea12d6c597de15e3e42f97ab483cfd069e103611200279a177e021e8e9c4e74387c4f45d2e58b3a1e7e2bdb32a1d2d2060b1f8086c03eeaa0c68579d9d54e
|
|
Shuffle function
6ecda04fefad980872c72fba89844393f5581120 random: drop ad-hoc Shuffle in favor of std::shuffle (Pieter Wuille)
da28a26aae3178fb7663efbe20bb650857ace775 bench random: benchmark more functions, and add InsecureRandomContext (Pieter Wuille)
0a9bbc64c157a314e5472ecd98300e30b12d3fdf random bench refactor: move to new bench/random.cpp (Pieter Wuille)
Pull request description:
This adds benchmarks for various operations on `FastRandomContext` and `InsecureRandomContext`, and then removes the ad-hoc `Shuffle` functions, now that it appears that standard library `std::shuffle` has comparable performance. The other reason for keeping `Shuffle`, namely the fact that libstdc++ used self-move (which debug mode panics on) has been fixed as well (see https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049).
ACKs for top commit:
achow101:
ACK 6ecda04fefad980872c72fba89844393f5581120
hodlinator:
ACK 6ecda04fefad980872c72fba89844393f5581120
dergoegge:
Code review ACK 6ecda04fefad980872c72fba89844393f5581120
Tree-SHA512: 2560b7312410581ff2b9bd0716e0f1558d910b5eadb9544785c972384985ac0f11f72d6b2797cfe2e7eb71fa57c30cffd98cc009cb4ee87a18b1524694211417
|
|
at trace level
46819f5df6de2a860c3ec87d55527b617a3b128f wallet: use LogTrace for walletdb log messages at trace level (Anthony Towns)
Pull request description:
Wallet sqlite logging is enabled by `-debug=walletdb -loglevel=walletdb:trace` however the actual log messages are sent at `BCLog::Level::Info`. Switch to the trace level to make this consistent. This adds `[walletdb:trace]` to the log output, eg:
```
[httpworker.3] [wallet/sqlite.cpp:55] [TraceSqlCallback] [/tmp/bitcoin_func_test_4fsnatpg/node0/regtest/wallets/boring/wallet.dat] SQLite Statement: BEGIN EXCLUSIVE TRANSACTION
```
becomes
```
[httpworker.0] [wallet/sqlite.cpp:55] [TraceSqlCallback] [walletdb:trace] [/tmp/bitcoin_func_test_9lcwth4z/node0/regtest/wallets/boring/wallet.dat] SQLite Statement: BEGIN EXCLUSIVE TRANSACTION
```
ACKs for top commit:
maflcko:
ACK 46819f5df6de2a860c3ec87d55527b617a3b128f
ryanofsky:
Code review ACK 46819f5df6de2a860c3ec87d55527b617a3b128f. Nice catch!
furszy:
ACK 46819f5df6de2a860c3ec87d55527b617a3b128f
luke-jr:
utACK 46819f5df6de2a860c3ec87d55527b617a3b128f
Tree-SHA512: 6fc1bc63c2ee686d4ca8f4f558f06c0cd9e7813b5fce1588351f55ef8bedfc23c97ea443e54a6a447008fa79ea022b6d631cb010929932f1db23fa8e255e6482
|
|
|
|
Benchmarks show it is no longer faster with modern standard C++ libraries,
and the debug-mode failure due to self-move has been fixed as well.
|
|
Set a max length for some previous
`ConsumeRandomLengthByteVector` usage.
|
|
Fix cases of calls to `FillPSBT` returning `complete=true` when it's not
the case.
This can happen when some inputs have been signed but the transaction is
subsequently modified, e.g. in the context of PayJoins.
Also fixes a related bug where a finalized hex string is attempted to be
added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort.
Reported in #30077.
|
|
In SetupLegacyScriptPubKeyMan, a base LegacyDataSPKM will be created if
the database has the format "bdb_ro" (i.e. the wallet was opened only
for migration purposes).
All of the loading functions are now called with a LegacyDataSPKM object
instead of LegacyScriptPubKeyMan.
|
|
|
|
IsMine is necessary for migration. It should be inlined with migration
when the legacy wallet is removed.
|
|
In order to load the necessary data for migrating a legacy wallet
without the full LegacyScriptPubKeyMan, move the data storage and
loading components to LegacyDataSPKM. LegacyScriptPubKeyMan now
subclasses that.
|
|
|
|
|
|
`CRecipient`s directly
a9c7300135f0188daa5bce5491e2daf2dd8da8ae move-only: refactor CreateTransactionInternal (josibake)
adc6ab25bba42ce9e7ed6bd7599aabb6dead6987 wallet: use CRecipient instead of CTxOut (josibake)
Pull request description:
Broken out from #28201
---
In order to estimate fees properly, we need to know what the final serialized transaction size will be. This PR refactors `CreateTransactionInternal` to:
* Get the serialized size directly from the `CRecipient`: this sets us up in a future PR to calculate the serialized size of silent payment `CTxDestinations` (see https://github.com/bitcoin/bitcoin/pull/28201/commits/797e21c8c1bf393e668eeef8bb7ee9cae1e5e41e)
* Use the new `GetSerializeSizeForRecipient` to move the serialize size calculation to *before* coin selection and the output creation to *after* coin selection: this also sets us up for silent payments sending in a future PR in that silent payments outputs cannot be created until after the inputs to the transaction have been selected
Aside from the silent payments use case, I think this structure logically makes more sense. As a reminder, move-only commits are best reviewed with something like `git diff -w --color-moved=dimmed-zebra`
ACKs for top commit:
S3RK:
reACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae
achow101:
ACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae
rkrux:
tACK [a9c7300](https://github.com/bitcoin/bitcoin/pull/30050/commits/a9c7300135f0188daa5bce5491e2daf2dd8da8ae)
Tree-SHA512: 412e1764b98f7428c8530c3a68f55e32063d6b66ab2ff613e1c7e12d49b049807cb60055cfe7f7e8ffe7ac7f0f9931427cbfd3efe7d4f97a5a0f6d1bf1aaac58
|
|
This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
|
|
and `tx_noinputs_size` to `int`
- This change ensures consistency in transaction size and weight calculation
within the wallet and prevents conversion overflow when calculating
`max_selection_weight`.
|
