8ed9002cd14165f751442f738fbf1fb8a37611b2 refactor: use local argsmanager in CRegTestParams (Ivan Metlushko)
9b20f6682845870d6ac53a01d3166fb83c467e7d scripted-diff: Replace gArgs with local argsman (Ivan Metlushko)
a316e9ce265212a7c6c4ef7922420f6ecba9e7b0 refactor: add unused ArgsManager to replace gArgs (Ivan Metlushko)
Pull request description:
Rationale: reduce use of gArgs to decouple code and simplify future maintenance and easier unit testing.
This PR is continuation of work started in #18926 and #18662
It covers only places that register args in ArgsManager with `AddArgs()` or `AddHiddenArgs()`.
Closes#19511
ACKs for top commit:
MarcoFalke:
ACK 8ed9002cd14165f751442f738fbf1fb8a37611b2 👛
Tree-SHA512: 7e6ba8e8357a48833c71e9c3942a769acb3d93bdcc6748a8ef2b7c4461a2499419b60896abf1d8b6bf8e88ee2590284cdd5da64220243ac22375300bcb8fe3e8
Previously having no database handle could still be considered a success
when BerkeleyDatabase and BerkeleyBatch were used for dummy database
things. With dedicated DummyDatabase and DummyBatch classes now, these
should fail.
d416ae560e46a4846a3fd5990b7d390d2ef30ec8 walletdb: Introduce WalletDatabase abstract class (Andrew Chow)
2179dbcbcd0b9bef7ad9c907b85294b9a1bccf0f walletdb: Add BerkeleyDatabase::Open dummy function (Andrew Chow)
71d28e7cdca1c8553531bb3a4725d7916363ec5c walletdb: Introduce AddRef and RemoveRef functions (Andrew Chow)
27b27663849932971eb5deadb1f19234b9cd97ea walletdb: Move BerkeleyDatabase::Flush(true) to Close() (Andrew Chow)
Pull request description:
A `WalletDatabase` abstract class is created from `BerkeleyDatabase` and is implemented by `BerkeleyDatabase`. First, to get to the point that this is possible, 4 functions need to be added to `BerkeleyDatabase`: `AddRef`, `RemoveRef`, `Open`, and `Close`.
First the increment and decrement of `mapFileUseCount` is refactored into separate functions `AddRef` and `RemoveRef`.
`Open` is introduced as a dummy function. This will raise an exception so that it always fails.
`Close` is refactored from `Flush`. The `shutdown` argument in `Flush` is removed and instead `Flush(true)` is now the `Close` function.
Split from #18971
Requires #19325
ACKs for top commit:
ryanofsky:
Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8. Only changes since last review were rebasing after base PR #19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids
fjahr:
Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8
meshcollider:
Code review & test run ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8
Tree-SHA512: 98d05ec093d7446c4488e2b0914584222a331e9a2f4d5be6af98e3f6d78fdd8e75526c12f91a8a52d4820c25bce02aa02aabe92d38bee7eb2fce07d0691b7b0d
1554b54d47d7e24ce2491f57d24e56d38ceb7649 Static asserts for consistency of fee defaults. (Daniel Kraft)
Pull request description:
This adds `static_assert`'s that ensure that the default values given for fee levels in the wallet (minimum fee and incremental feerate increase) are at least as high as the corresponding levels configured in the core node policy. Since the core policy values are enforced by the network, it makes sense for the wallet to be conservative and above (or at least not below) this.
ACKs for top commit:
laanwj:
code review ACK 1554b54d47d7e24ce2491f57d24e56d38ceb7649, these assumptions seem straightforward
Tree-SHA512: 50e5adf082f467062334377f82a3ee75bcfd436afc65bd0eb33c8d0549d6d90fd1f48c31f60cabe523eb59be9efa8ae0879e9e09cd51ca9c1bd466631ce03cf4
d0ea9bab2804928c9f40def61fd99064d2d8f9b8 walletdb: Don't remove database transaction logs and instead error (Andrew Chow)
Pull request description:
Instead of removing the database transaction logs and retrying the
wallet loading, just return an error message to the user. Additionally,
speciically for DB_RUNRECOVERY, notify the user that this could be due
to different BDB versions.
Kind of implements the suggestion from https://github.com/bitcoin/bitcoin/pull/18870#discussion_r421647964
ACKs for top commit:
Sjors:
re-utACK d0ea9bab2804928c9f40def61fd99064d2d8f9b8
ryanofsky:
Code review ACK d0ea9bab2804928c9f40def61fd99064d2d8f9b8. Only changes since last review are rebase and expanding error and commit messages.
Tree-SHA512: f6e67dc70f58188742a5c8af7cdc63a2b58779aa0d26ae7f1e75805a239f1a342433860e5a238d6577fae5ab04b9d15e7f11c55b867065dfd13781a6a62e4958
b82f0ca4d5465b36debb6c57f335bdccf4899c49 walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow)
eac9200814fa01da6522625be01dded730b26751 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow)
Pull request description:
In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`.
The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`.
Part of #18971
Requires #19308 and #19324
ACKs for top commit:
Sjors:
re-utACK b82f0ca4d5465b36debb6c57f335bdccf4899c49
MarcoFalke:
ACK b82f0ca4d5465b36debb6c57f335bdccf4899c49 🌘
meshcollider:
LGTM, utACK b82f0ca4d5465b36debb6c57f335bdccf4899c49
Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a
Instead of removing the database transaction logs and retrying the
wallet loading, just return an error message to the user. Additionally,
specifically for DB_RUNRECOVERY, notify the user that this could be due
to different BDB versions. This error is pretty much only caused by
compiling with a newer version of BDB and then trying to open the wallet
with a version compiled with an older version of BDB.
08fc6f6cfc3b06fd170452a766696d7b833113fa [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost)
Pull request description:
I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination.
Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this.
Salvaged from #18201.
ACKs for top commit:
fjahr:
Code review ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
jonatack:
ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
meshcollider:
Code review & functional test run ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909
facd7dd3d1f9d51e1133974ff69eeb48f5ae282b wallet: Fix typo in comments; Simplify assert (MarcoFalke)
Pull request description:
Follow up to https://github.com/bitcoin/bitcoin/pull/19046#discussion_r443783101 and https://github.com/bitcoin/bitcoin/pull/19046#discussion_r443793690
ACKs for top commit:
practicalswift:
ACK facd7dd3d1f9d51e1133974ff69eeb48f5ae282b
jonatack:
ACK facd7dd3d1f9d51e1133974ff69eeb48f5ae282b
hebasto:
ACK facd7dd3d1f9d51e1133974ff69eeb48f5ae282b, spelling verified with `test/lint/lint-spelling.sh`: all remaining warnings are false positive.
Tree-SHA512: 2b185d138058840db56726bb6bcc42e5288a954e2a410c49e04806a047fbbdaf0bb2decc70ecf7613c69caa766655705ca44151613e7ea5015b386d1e726d870
1e58bcc9afefcf009653567c6373b4f7facba8f5 wallet: Fix clang build in Mac (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Top commit has no ACKs.
Tree-SHA512: 19312929af14dab97c37cf4547fbd6589a6de960f1a499c2118bb684240639af4b127cf8dc4d201b41d253cfbb645614a0606d4ecce29f300b10c210d38a961b
a66a7a1a7060bb422eba3b8c214852416c4280d1 walletdb: don't reinitialize desc cache with multiple cache entries (Andrew Chow)
Pull request description:
When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works.
ACKs for top commit:
hugohn:
tACK [a66a7a1](a66a7a1a70)
jonatack:
ACK a66a7a1a706
meshcollider:
Code review ACK a66a7a1a7060bb422eba3b8c214852416c4280d1
Tree-SHA512: 3df746421a008708eaa3bbbdd12b9ddd3e2ec111d54625a212dca7414b971cc1f6e2b1757b3232c31a2f637d1b1ef43bf3ffa4ac4216646cf1e92db5f79954f1
fa73493930e35850e877725167dc9d42e47015c8 refactor: Use C++11 range-based for loop (MarcoFalke)
fa7b164d62d9f12e9cda79bf28bf435acf2d1e38 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off (MarcoFalke)
faf8401c195f52470d1ca6e2c94cb3820e57ee41 wallet: Pass unused args to StartWallets (MarcoFalke)
fa6c186436337c8ed7d9e1ab065377f8cda5c0b7 gui tests: Limit life-time of dummy testing setup (MarcoFalke)
fa28a618972911239a119248ab1194702a5c36d8 test: Add smoke test to check that wallets are flushed by default (MarcoFalke)
Pull request description:
User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second.
ACKs for top commit:
jnewbery:
utACK fa73493930e35850e877725167dc9d42e47015c8
meshcollider:
utACK fa73493930e35850e877725167dc9d42e47015c8
ryanofsky:
Code review ACK fa73493930e35850e877725167dc9d42e47015c8. Just rebased since last review
Tree-SHA512: 99e1fe1b2c22a3f4b19de3e566241d38693f4fd8d5a68ba1838d86740aa6c08e3325c11a072e30fd262a8861af4278bed52eb9374c85179b8f536477f528247c
3a9aba21a49a6d80bd187940d5e26893937b6832 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow)
d9cd095b5965fc20c09f401370e7ba99446663e3 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow)
0122fbab4c340b23ae56173de6c5ab866ba25ab8 Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow)
Pull request description:
`SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in https://github.com/bitcoin/bitcoin/pull/17681#discussion_r427633081. This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet.
`AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet.
`LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk.
ACKs for top commit:
jnewbery:
Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832
ryanofsky:
Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832. Only changes since last review tweaks making m_wallet_flags updates more safe
meshcollider:
utACK 3a9aba21a49a6d80bd187940d5e26893937b6832
Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
9c59f9c285303659ee1beed7555bbb322e6e6981 Fix ZapSelectTx to sync wallet spends (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
ACKs for top commit:
achow101:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981
ryanofsky:
Code review ACK 9c59f9c285303659ee1beed7555bbb322e6e6981. Only change since last review tweaking the for loop as suggested
jonatack:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981 tested rebased on current master b33136b6ba9887f7d and the new unit test does indeed fail without the change.
meshcollider:
utACK 9c59f9c285303659ee1beed7555bbb322e6e6981
Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
fa8a341b88cabfd7f8d702db7cb9972b0804bf2a wallet: Replace CDataStream& with CDataStream&& where appropriate (MarcoFalke)
fa021e9a5b7e930a3db0febb416942dea3a90a8f wallet: Remove confusing double return value ret+success (MarcoFalke)
Pull request description:
The keys and values are only to be used once because their memory is set
to zero. Make that explicit by moving the bytes into the lower level
methods.
ACKs for top commit:
sipa:
utACK fa8a341b88cabfd7f8d702db7cb9972b0804bf2a
ryanofsky:
Code review ACK fa8a341b88cabfd7f8d702db7cb9972b0804bf2a. Nice changes.
Tree-SHA512: 5c0218bae0f3cd2a07346f1bbf4ad232e5dde7ef2f807d82cc6cfd208d11fe60c8b0f37e7986087b52fbfc79cdfd33c3c8a5822b3d4d9a44d1c6b09e354fc424
d8e9ca66d119d80acfb2bb3c8940c386ce0fc226 walletdb: Move Rewrite into BerkeleyDatabase (Andrew Chow)
91d109156d63ff81cda534bd7bec8369af0027dd walletdb: Move PeriodicFlush into WalletDatabase (Andrew Chow)
8f1bcf8b7b6e47c05f2e43dd98ec3505b888d8b3 walletdb: Combine VerifyDatabaseFile and VerifyEnvironment (Andrew Chow)
Pull request description:
The `BerkeleyBatch` class has 4 static functions that operate on `BerkeleyDatabase` or `BerkeleyEnvironment`. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from `BerkeleyBatch` into `BerkeleyDatabase` and make them member functions instead of static.
`BerkeleyBatch::VerifyEnvironment` and `BerkeleyBatch::VerifyDatabaseFile` are combined into a single `BerkeleyDatabase::Verify` function that operates on that `BerkeleyDatabase` object.
`BerkeleyBatch::Rewrite` and `BerkeleyBatch::PeriodicFlush` both took a `BerkeleyDatabase` as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.
Part of #18971
ACKs for top commit:
MarcoFalke:
re-ACK d8e9ca66d1 only change is test fixup 🤞
promag:
Code review ACK d8e9ca66d119d80acfb2bb3c8940c386ce0fc226, good stuff.
Tree-SHA512: 9847e55b13d98bf4e5636cc14bc3f5351d56737f7e320fafffaed128606240765599e5400382c5aecac06690f7e36265ca3e1031f3f6d8a9688f6d5cb1bacd2a
fab80fef61ddd4afeff6e497c7e76bffcd05e8a4 refactor: Remove unused EnsureChainman (MarcoFalke)
fa34587f1c811d99200453b0936219c473f514b0 scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke)
fa6ef701adba1cb48535cac25fd43c742a82e40d util: Add Assert identity function (MarcoFalke)
fa457fbd3387661e1973a8f4e5cc2def79e0c625 move-only: Move NDEBUG compile time check to util/check (MarcoFalke)
Pull request description:
The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated.
ACKs for top commit:
promag:
Tested ACK fab80fef61ddd4afeff6e497c7e76bffcd05e8a4.
ryanofsky:
Code review ACK fab80fef61ddd4afeff6e497c7e76bffcd05e8a4
Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
When loading descriptor caches, we would accidentally reinitialize the
descriptor cache when seeing that one already exists. This should have
only been initializing the cache when one does not exist. However this
code itself is unnecessary as the act of looking up the cache to add to
it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a
multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case.
Another test case has also been added to check that loading a wallet
with only single key descriptors works.
fa0dfdf447d5b84a1849dc823d8508463600136a refactor: Remove confusing BlockIndex global (MarcoFalke)
Pull request description:
The global `::BlockIndex()` is problematic for several reasons:
* It returns a mutable reference to the block tree, without the appropriate lock annotation (`m_block_index` is guarded by `cs_main`). The current code is fine, but in the future this might lead to accidental races and data corruption.
* The rpc server shouldn't rely on node globals, but rather a context that is passed in to the RPC method.
* Tests might want to spin up their own block tree, and thus should also not rely on a single global.
Fix all issues by removing the global
ACKs for top commit:
promag:
Code review ACK fa0dfdf447d5b84a1849dc823d8508463600136a.
jonatack:
re-ACK fa0dfdf
Tree-SHA512: 8f158fc5e1c67e73588a21c25677b3fa0fe442313b13ec24b87054806c59607d6ba0c062a865ce3e0ee568706bd0d1faa84febda21aff5bcd65dab172f74c52f
84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow)
46004790588c24174a0bec49b540d158ce163ffd psbt: always put a non_witness_utxo and don't remove it (Andrew Chow)
5279d8bc07d601fe6a67ad665fbc7591fe73c7de psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow)
72f6bec1da198764d4648a10a61c485e7ab65e9e rpc: show both UTXOs in decodepsbt (Andrew Chow)
Pull request description:
Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a `witness_utxo` to determine whether to produce a segwit signature, we keep that field to ease the transition.
Because all of the sanity checks implemented by the `IsSane` functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper.
Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs.
As discussed in the wallet IRC meeting, our own signer will not require `non_witness_utxo` for segwit inputs.
ACKs for top commit:
Sjors:
utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators)
ryanofsky:
Code review re-ACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested https://github.com/bitcoin/bitcoin/pull/19215#discussion_r447889473 and maybe refer to
meshcollider:
utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3
Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940
fa927ff884ae373c676deed63180a8d238872cdc Enable Wswitch for OutputType (MarcoFalke)
faddad71f648ed99734f4f8811bd4bc7232ca670 Remove confusing OutputType::CHANGE_AUTO (MarcoFalke)
fa2eb383522249a5f4d48726c520cec5de496614 interfaces: Remove unused getDefaultChangeType (MarcoFalke)
Pull request description:
`OutputType::CHANGE_AUTO` is problematic for several reasons:
* An output that is not change must never be described by `CHANGE_AUTO`. Simply allowing that option makes the code confusing and review harder than it needs to be.
* To make review even harder, `CHANGE_AUTO` requires `-Wswitch` to be disabled for `OutputType`
Fix both issues by removing `CHANGE_AUTO` and then enabling `-Wswitch` for `OutputType`
ACKs for top commit:
promag:
Code review ACK fa927ff884ae373c676deed63180a8d238872cdc.
laanwj:
Code review ACK fa927ff884ae373c676deed63180a8d238872cdc
Tree-SHA512: 24fd809757aa343866c94dafe9a7130b50cda4f77c97666d407f99b813f75b115a7d8e688a6bc2a737e87cba64ddd4e43f2b3c5538fd35fabb5845807bb39134
ca24edfbc1941ed0a3c9586416dae4e84794eb66 walletdb: Handle cursor internally (Andrew Chow)
Pull request description:
Instead of returning a Dbc (BDB cursor object) and having the caller deal with the cursor, make BerkeleyBatch handle the cursor internally.
Split from #18971
ACKs for top commit:
ryanofsky:
Code review ACK ca24edfbc1941ed0a3c9586416dae4e84794eb66. Changes since last review: StartCursor rename, moving CloseCursor calls near returns
promag:
Code review ACK ca24edfbc1941ed0a3c9586416dae4e84794eb66.
Tree-SHA512: f029b498c7f275aedca53ce7ade7cb99c82975fd6cad17346a4990fb3bcc54e2a5309b32053bd13def9ee464d331b036ac79abb8fc4fa561170c6cfc85283447
