As is, this sanity check doesn't seem to be testing fdelt_chk, because
passing a value of "0" to FD_SET wont cause the compiler to insert any
calls to fdelt_chk().
The documentation is a little misleading. If we actually triggered fdelt_chk
at runtime, bitcoind would abort. I think this check would be better replaced
(if possible) by additional checks in security-check.py.
The compiler may insert a call to fdelt_warn() (aliased with fdelt_chk
in glibc) at compile time if it can determine that an invalid value is
being passed to FD_SET.
These checks are essentially; value < 0 or value >= FD_SETSIZE along
with a check for wether the value is a compile time constant.
If the compiler can determine an invalid value is being passed, a call
to fdelt_warn will be inserted. Passing 0 should never cause a call to
be inserted.
You can check this after compiling:
```bash
objdump -dC bitcoind | grep sanity_fdelt
...
0000000000399d20 <sanity_test_fdelt()>:
399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
399d27: b9 10 00 00 00 mov $0x10,%ecx
399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
399d33: 00 00
399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
399d3c: 00
399d3d: 31 c0 xor %eax,%eax
399d3f: 48 89 e7 mov %rsp,%rdi
399d42: fc cld
399d43: f3 48 ab rep stos %rax,%es:(%rdi)
399d46: 48 8b 84 24 88 00 00 mov 0x88(%rsp),%rax
399d4d: 00
399d4e: 64 48 33 04 25 28 00 xor %fs:0x28,%rax
399d55: 00 00
399d57: 75 0d jne 399d66 <sanity_test_fdelt()+0x46>
399d59: b8 01 00 00 00 mov $0x1,%eax
399d5e: 48 81 c4 98 00 00 00 add $0x98,%rsp
399d65: c3 retq
399d66: e8 85 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
399d6b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
```
To test, you could modify this test to pass -1 to FD_SET, and check
that a call to fdelt_warn() is inserted, and that running bitcoind
fails. i.e:
```bash
0000000000399d20 <sanity_test_fdelt()>:
399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
399d27: b9 10 00 00 00 mov $0x10,%ecx
399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
399d33: 00 00
399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
399d3c: 00
399d3d: 31 c0 xor %eax,%eax
399d3f: 48 89 e7 mov %rsp,%rdi
399d42: fc cld
399d43: f3 48 ab rep stos %rax,%es:(%rdi)
399d46: 48 c7 c7 ff ff ff ff mov $0xffffffffffffffff,%rdi
399d4d: e8 3e ff ff ff callq 399c90 <__fdelt_warn>
399d52: 0f b6 04 24 movzbl (%rsp),%eax
399d56: 83 e0 01 and $0x1,%eax
399d59: 48 8b 94 24 88 00 00 mov 0x88(%rsp),%rdx
399d60: 00
399d61: 64 48 33 14 25 28 00 xor %fs:0x28,%rdx
399d68: 00 00
399d6a: 75 08 jne 399d74 <sanity_test_fdelt()+0x54>
399d6c: 48 81 c4 98 00 00 00 add $0x98,%rsp
399d73: c3 retq
399d74: e8 77 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
399d79: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
```
```bash
./src/bitcoind
*** buffer overflow detected ***: src/bitcoind terminated
Aborted
```
748977690e Add asmap_direct fuzzer that tests Interpreter directly (Pieter Wuille)
7cf97fda15 Make asmap Interpreter errors fatal and fuzz test it (Pieter Wuille)
c81aefc537 Add additional effiency checks to sanity checker (Pieter Wuille)
fffd8dca2d Add asmap sanity checker (Pieter Wuille)
5feefbe6e7 Improve asmap Interpret checks and document failures (Pieter Wuille)
2b3dbfa5a6 Deal with decoding failures explicitly in asmap Interpret (Pieter Wuille)
1479007a33 Introduce Instruction enum in asmap (Pieter Wuille)
Pull request description:
This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow.
In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file.
I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it.
ACKs for top commit:
practicalswift:
ACK 748977690e modulo feedback below.
jonatack:
ACK 748977690e code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight.
fjahr:
ACK 748977690e
Tree-SHA512: d876df3859735795c857c83e7155ba6851ce839bdfa10c18ce2698022cc493ce024b5578c1828e2a94bcdf2552c2f46c392a251ed086691b41959e62a6970821
fa47cf9d95 wallet: Fix typo in assert that is compile-time true (MarcoFalke)
Pull request description:
Commit 92bcd70808 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`.
However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb
ACKs for top commit:
Sjors:
utACK fa47cf9d95
Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
71f183a49b build: warn on potentially uninitialized reads (Vasil Dimov)
Pull request description:
* Enable `conditional-uninitialized` warning class to show potentially uninitialized
reads.
* Fix the sole such warning in Bitcoin Core in `GetRdRand()`: `r1` would be
set to `0` on `rdrand` failure, so initializing it to `0` is a non-functional
change.
ACKs for top commit:
practicalswift:
ACK 71f183a49b
laanwj:
ACK 71f183a49b
Tree-SHA512: 2c1d8caacd86424b16a9d92e5df19e0bedb51ae111eecad7e3bfa46447bc88e5fff1f32dacf6c4a28257ebb3d87e79f80f074ce2c523ce08b1a0c0a67ab44204
fa09110ebb doc: Fix typo in Coin doxygen comment (MarcoFalke)
Pull request description:
`CTxOutCompressor` has been renamed in commit 4de934b9b5, so rename it in the docs as well.
ACKs for top commit:
laanwj:
ACK fa09110ebb
hebasto:
ACK fa09110ebb
Tree-SHA512: e16a21ac3112a67ee7d5ffabb3f47103aed8f91fdebf1bf96311cd0b7bdb9b7323ed826bfa95517386d4128ff0ae2c7c13bad047a7c5a0cc2458be7a43119157
1ad8ea2b73 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner)
Pull request description:
The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters.
However, the real reason of adding those flags (introduced with commit 37c6389c5a by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash.
According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165):
> the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :)
For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html).
The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix.
ACKs for top commit:
meshcollider:
utACK 1ad8ea2b73
laanwj:
Concept and code review ACK 1ad8ea2b73
jkczyz:
ACK 1ad8ea2b73
MarcoFalke:
ACK 1ad8ea2b73
fjahr:
Code review ACK 1ad8ea2b73
Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
28b112e9bd Get rid of BindWallet (Russell Yanofsky)
d002f9d15d Disable CWalletTx copy constructor (Russell Yanofsky)
65b9d8f8dd Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky)
bd2fbc7cdb Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky)
2b9cba2065 Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky)
Pull request description:
This is a pure refactoring, no behavior is changing.
Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly.
This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them.
This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged.
Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function.
This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying.
ACKs for top commit:
MarcoFalke:
Anyway, re-ACK 28b112e9bd
Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef
m_address_type was used for two things:
1. Determine the type of descriptor to generate during
SetupDescriptorGeneration
2. Sanity check during GetNewDestination.
There is no need to have this variable to accomplish those things.
1. Add a argument to SetupDescriptorGeneration indicating the address
type to use
2. Use Descriptor::GetOutputType for the sanity check.
2a78098098 wallet: Make sure no WalletDescriptor members are uninitialized after construction (practicalswift)
ff046aeeba wallet: Make sure no DescriptorScriptPubKeyMan members are uninitialized after construction (practicalswift)
Pull request description:
This is a small folllow-up to #16528 ("Native Descriptor Wallets using DescriptorScriptPubKeyMan") which was merged in to `master` a couple of hours ago.
Make sure no `DescriptorScriptPubKeyMan` or `WalletDescriptor` members are left uninitialized after construction.
Before this change `bool m_internal` was left uninitialized when using the `DescriptorScriptPubKeyMan(WalletStorage&, WalletDescriptor&)` ctor.
The same goes for the now initialized integers which were left uninitialized when using the `WalletDescriptor()` ctor.
ACKs for top commit:
instagibbs:
utACK 2a78098098
fjahr:
Code review ACK 2a78098098
Sjors:
utACK 2a78098
achow101:
ACK 2a78098098
brakmic:
Code review ACK 2a78098098
meshcollider:
utACK 2a78098098
Tree-SHA512: c98e035268fdc7f65a423b73ac0cf010b0ef7c5e679b3cf170c1813efac8ab5c657dcbaf43c746770bea59e4772bfefe4caa834f1175260c39c7f35d92946ba5
0ae8f18dfe build: add -Wgnu to compile flags (fanquake)
3a0fd7726b Remove use of non-standard zero variadic macros (Ben Woosley)
49f6178c3e Drop unused LOG_TIME_MICROS helper (Ben Woosley)
5d4999951e prevector: Avoid unnamed struct, which is a GNU extension (DesWurstes)
Pull request description:
Since we [started using](https://github.com/bitcoin/bitcoin/pull/7165) the `ax_cxx_compile_stdcxx.m4` macro we've been passing `[noext]` to indicate that we don't want to use an extended mode, i.e GNU extensions. Speaking to Cory he clarified that the intention was to "require only vanilla c++11 and turn _off_ extension support so they would fail to compile".
However in the codebase we are currently making use of some GNU extensions. We should either remove there usage, or at least amend our CXX compiler checks. I'd prefer the former.
#### anonymous structs
```bash
./prevector.h:153:9: warning: anonymous structs are a GNU extension [-Wgnu-anonymous-struct]
struct {
```
This is fixed in b849212c1e.
#### variadic macros
```bash
./undo.h:57:50: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]
::Unserialize(s, VARINT(nVersionDummy));
```
This is taken care of in #18087.
The `LOG_TIME_*` macros introduced in #16805 make use of a [GNU extension](https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html).
```bash
In file included from validation.cpp:22:
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:101:92: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
6 warnings generated.
```
This is fixed in 081a0ab64eb442bc85c4d4a4d3bc2c8e97ac2a6d and 612e8e138b97fc5ad2f38847300132a8fc423c3f.
#### prevention
To ensure that usage doesn't creep back in we can add [`-Wgnu`](https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu) to our compile time flags, which will make Clang warn whenever it encounters GNU extensions.
This would close #14130.
Also related to #17230, where it's suggested we use a GNU extension, the `gnu::pure` attribute.
ACKs for top commit:
practicalswift:
ACK 0ae8f18dfe -- diff looks correct
MarcoFalke:
ACK 0ae8f18dfe
vasild:
utACK 0ae8f18df
dongcarl:
ACK 0ae8f18dfe
Tree-SHA512: c517404681ef8edf04c785731d26105bac9f3c9c958605aa24cbe399c649e7c5ee0c4aa8e714fd2b2d335e2fbea4d571e09b0dec36678ef871f0a6683ba6bb7f
f85203097f lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov)
Pull request description:
This is a followup to
23991ee53 / https://github.com/bitcoin/bitcoin/pull/15600
to also use madvise(2) on FreeBSD to avoid sensitive data allocated
with secure_allocator ending up in core files in addition to preventing
it from going to the swap.
ACKs for top commit:
sipa:
ACK f85203097f if someone verifies this works as intended on *BSD.
laanwj:
ACK f85203097f
practicalswift:
Code-review ACK f85203097f assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :)
Tree-SHA512: 2e6d4ab6a9fbe18732c8ba530eacc17f58128c97140758b80c905b5b838922a2bcaa5f9abc45ab69d5a1a2baa0cba322f006048b60a877228e089c7e64dadd2a
fa2cce4391 wallet: Remove trailing whitespace from potential translation strings (MarcoFalke)
fa59cc1c97 wallet: Report full error message in wallettool (MarcoFalke)
fae7776690 wallet: Avoid translating RPC errors when creating txs (MarcoFalke)
fae51a5c6f wallet: Avoid translating RPC errors when loading wallets (MarcoFalke)
Pull request description:
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
`CreateWalletFromFile` does not know its caller, so this commit changes it
to return a `bilingual_str` to the caller.
Fixes #17072
ACKs for top commit:
laanwj:
ACK fa2cce4391, checked that no new translation messages are added compared to master.
hebasto:
ACK fa2cce4391
Tree-SHA512: c6a943ae9c3689ea3c48c20d26de6e4970de0257a1f1eec57a2bded67a4af9dcc5c45b2d64659d6fb4c4bc4d8103e28483ea3d14bb850df8db0ff9e8e5c77ee2
faec3dc2ad init: Remove boost from ThreadImport (MarcoFalke)
Pull request description:
Can be tested by calling `-reindex` or `-loadblock` and then pressing `CTRL`+`C`.
Should print something like:
```
...
2020-04-27T19:34:31Z [loadblk] Reindexing block file blk00005.dat...
^C2020-04-27T19:34:32Z [loadblk] Shutdown requested. Exit ThreadImport
2020-04-27T19:34:32Z [qt-init] Interrupting HTTP server
...
```
ACKs for top commit:
laanwj:
Code review ACK faec3dc2ad
hebasto:
ACK faec3dc2ad, tested on Linux Mint 19.3 (x86_64) both `bitcoind` and `bitcoin-qt` binaries.
Tree-SHA512: e105af18d98296d82ec99f48e478cf44577e3c32f7e4b47617a7bc7cbf71d6becb92722f229a1be38d58ad29712704509ad9740d8ab8cd3104cf90057664b437
38e49ded8b tests: Add fuzzing harness for MessageSign, MessageVerify and other functions in util/message.h (practicalswift)
Pull request description:
Add fuzzing harness for `MessageSign`, `MessageVerify` and other functions in `util/message.h`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
vasild:
utACK 38e49ded8b
Tree-SHA512: 4f83718365d9c7e772a4ccecb31817bf17117efae2bfaf6e9618ff17908def0c8b97b5fa2504d51ab38b2e6f82c046178dd751495cc37ab4779c0b1ac1a4d211
b56607a89b Remove CCoinsViewCache::GetValueIn(...) (practicalswift)
Pull request description:
Remove `CCoinsViewCache::GetValueIn(...)`.
Fixes #18858.
It seems like `GetValueIn` was added in #748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in #8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017).
`CCoinsViewCache::GetValueIn(…)` performs money summation like this:
```c++
CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
{
if (tx.IsCoinBase())
return 0;
CAmount nResult = 0;
for (unsigned int i = 0; i < tx.vin.size(); i++)
nResult += AccessCoin(tx.vin[i].prevout).out.nValue;
return nResult;
}
```
Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow.
Proof of concept output:
```
coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long'
GetValueIn = -9221444073709551616
```
Proof of concept code:
```c++
CMutableTransaction mutable_transaction;
mutable_transaction.vin.resize(4393);
Coin coin;
coin.out.nValue = MAX_MONEY;
assert(MoneyRange(coin.out.nValue));
CCoinsCacheEntry coins_cache_entry;
coins_cache_entry.coin = coin;
coins_cache_entry.flags = CCoinsCacheEntry::DIRTY;
CCoinsView backend_coins_view;
CCoinsViewCache coins_view_cache{&backend_coins_view};
CCoinsMap coins_map;
coins_map.emplace(COutPoint{}, std::move(coins_cache_entry));
coins_view_cache.BatchWrite(coins_map, {});
const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction});
std::cout << "GetValueIn = " << total_value_in << std::endl;
```
ACKs for top commit:
MarcoFalke:
ACK b56607a89b
promag:
Code review ACK b56607a89b.
jb55:
ACK b56607a89b
hebasto:
ACK b56607a89b, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
f5a3a5b9ab gui: Add close window shortcut (Miguel Herranz)
Pull request description:
CMD+W is the standard shortcut in macOS to close a window without
exiting the program.
This adds support to use the shortcut in both main and debug windows.
ACKs for top commit:
jonasschnelli:
Tested ACK f5a3a5b9ab
hebasto:
ACK f5a3a5b9ab, tested on Linux Mint 19.3 by manually opening available dialogs and sub-windows, and applying the `Ctrl+W` shortcut. Also tested with "Minimize on close" option enabled / disabled.
Tree-SHA512: 39851f6680cf97c334d5759c6f8597cb45685359417493ff8b0566672edbd32303fa15ac4260ec8ab5ea1458a600a329153014f25609e1db9cf399aa851ae2f9
Enable -Wconditional-uninitialized to warn on potentially uninitialized
reads.
Fix the sole such warning in Bitcoin Core in GetRdRand(): r1 would be
set to 0 on rdrand failure, so initializing it to 0 is a non-functional
change.
From "Intel 64 and IA-32 ArchitecturesSoftware Developer's Manual" [1],
page 1711: "CF=1 indicates that the data in the destination is valid.
Otherwise CF=0 and the data in the destination operand will be returned
as zeros for the specified width."
[1] https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
Also, mark feebumper bilingual_str as Untranslated
They are technical and have previously not been translated either.
It is questionable whether they can even appear in the GUI.
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.
This reverts commit 0933a37078 from
https://github.com/bitcoin/bitcoin/pull/18160 which no longer an optimization
since commit "gui: Avoid wallet tryGetBalances calls before TransactionChanged
or BlockTip notifications".
6a72f26968 [wallet] Remove locked_chain from CWallet, its RPCs and tests (Antoine Riard)
841178820d [wallet] Move methods from Chain::Lock interface to simple Chain (Antoine Riard)
0a76287387 [wallet] Move getBlockHash from Chain::Lock interface to simple Chain (Antoine Riard)
de13363a47 [wallet] Move getBlockHeight from Chain::Lock interface to simple Chain (Antoine Riard)
b855592d83 [wallet] Move getHeight from Chain::Lock interface to simple Chain (Antoine Riard)
Pull request description:
This change is intended to make the bitcoin node and its rpc, network and gui interfaces more responsive while the wallet is in use. Currently, because the node's `cs_main` mutex is always locked before the wallet's `cs_wallet` mutex (to prevent deadlocks), `cs_main` currently stays locked while the wallet does relatively slow things like creating and listing transactions.
Switching the lock order so `cs_main` is acquired after `cs_wallet` allows `cs_main` to be only locked intermittently while the wallet is doing slow operations, so the node is not blocked waiting for the wallet.
To review the present PR, most of getting right the move is ensuring any `LockAssertion` in `Chain::Lock` method is amended as a `LOCK(cs_main)`. And in final commit, check that any wallet code which was previously locking the chain is now calling a method, enforcing the lock taking job. So far the only exception I found is `handleNotifications`, which should be corrected.
ACKs for top commit:
MarcoFalke:
re-ACK 6a72f26968🔏
fjahr:
re-ACK 6a72f26968
ryanofsky:
Code review ACK 6a72f26968. Only difference compared to the rebase I posted is reverting unneeded SetLastBlockProcessed change in wallet_disableprivkeys test
Tree-SHA512: 9168b3bf3432d4f8bc4d9fa9246ac057050848e673efc264c8f44345f243ba9697b05c22c809a79d1b51bf0de1c4ed317960e496480f8d71e584468d4dd1b0ad
interfaces::Wallet::tryGetBalances was recently updated in
https://github.com/bitcoin/bitcoin/pull/18160 to avoid computing balances
internally, but this not efficient as it could be with #10102 because
tryGetBalances is an interprocess call.
Implementing the TransactionChanged / BlockTip check outside of tryGetBalances
also allows tryGetBalances to be simplified in next commit 'Revert "gui: Avoid
Wallet::GetBalance in WalletModel::pollBalanceChanged"'.
