5edad5ce5d test: add -getinfo multiwallet functional tests (Jon Atack)
903b6c117f rpc: drop unused JSONRPCProcessBatchReply size arg, refactor (Jon Atack)
afce85eb99 cli: use GetWalletBalances() functionality for -getinfo (Jon Atack)
9f01849a49 cli: create GetWalletBalances() to fetch multiwallet balances (Jon Atack)
743077544b cli: lift -rpcwallet logic up to CommandLineRPC() (Jon Atack)
29f2cbdeb7 cli: extract connection exception handler, -rpcwait logic (Jon Atack)
Pull request description:
This PR is a client-side version of #18453, per review feedback there and [review club discussions](https://bitcoincore.reviews/18453#meeting-log). It updates `bitcoin-cli -getinfo` on the client side to display wallet name and balance for the loaded wallets when more than one is loaded (e.g. you are in "multiwallet mode") and `-rpcwallet=` is not passed; otherwise, behavior is unchanged.
before
```json
$ bitcoin-cli -getinfo -regtest
{
"version": 199900,
"blocks": 15599,
"headers": 15599,
"verificationprogress": 1,
"timeoffset": 0,
"connections": 0,
"proxy": "",
"difficulty": 4.656542373906925e-10,
"chain": "regtest",
"balance": 0.00001000,
"relayfee": 0.00001000
}
```
after
```json
$ bitcoin-cli -getinfo -regtest
{
"version": 199900,
"blocks": 15599,
"headers": 15599,
"verificationprogress": 1,
"timeoffset": 0,
"connections": 0,
"proxy": "",
"difficulty": 4.656542373906925e-10,
"chain": "regtest",
"balances": {
"": 0.00001000,
"Encrypted": 0.00003500,
"day-to-day": 0.00000120,
"side project": 0.00000094
}
}
```
-----
`Review club` discussion about this PR is here: https://bitcoincore.reviews/18453
This PR can be manually tested by building, creating/loading/unloading several wallets with `bitcoin-cli createwallet/loadwallet/unloadwallet` and running `bitcoin-cli -getinfo` and `bitcoin-cli -rpcwallet=<wallet-name> -getinfo`.
`wallet_multiwallet.py --usecli` provides regression test coverage on this change, along with `interface_bitcoin_cli.py` where this PR adds test coverage.
Credit to Wladimir J. van der Laan for the idea in https://github.com/bitcoin/bitcoin/issues/17314 and https://github.com/bitcoin/bitcoin/pull/18453#issuecomment-605431806.
ACKs for top commit:
promag:
Tested ACK 5edad5ce5d.
jnewbery:
utACK 5edad5ce5d
meshcollider:
Code review ACK 5edad5ce5d
Tree-SHA512: 4ca36c5f6c49936b40afb605c44459c1d5b80b5bd84df634007ca276b3f6c102a0cb382f9d528370363ee32c94b0d7ffa15184578eaf8de74179e566c5c5cee5
fab6b9d18f validation: Mark g_chainman DEPRECATED (MarcoFalke)
fa1d97b256 validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke)
fa24d49098 validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke)
fa84b1cd84 validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke)
fa05fdf0f1 net: Pass chainman into PeerLogicValidation (MarcoFalke)
fa7b626d7a node: Add chainman alias for g_chainman (MarcoFalke)
Pull request description:
The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.
The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.
I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.
ACKs for top commit:
ryanofsky:
Code review ACK fab6b9d18f. Had to be rebased but still looks good
Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
ea9fcfd130 doc: Drop protobuf stuff (Hennadii Stepanov)
Pull request description:
This is a follow-up to #17165.
ACKs for top commit:
fanquake:
ACK ea9fcfd130 - clicked the links and they seem to work.
Tree-SHA512: 0861bbac3a3ff781a413e15f5ed02c624bc15d572a001a53cd2fb9f7683456175f69e9d666b72f260abbb5114b67cefca9fada4d179c62384c90479534ae63d5
6a239e72eb tests: Don't limit fuzzing inputs to 1 MB for afl-fuzz (now: ∞ ∀ fuzzers) (practicalswift)
Pull request description:
Don't limit fuzzing inputs to 1 MB for `afl-fuzz`.
This change provides a level playing field for all fuzzers which allows for fair benchmarking using projects such as the excellent [FuzzBench](https://github.com/google/fuzzbench) project.
Prior to this commit we limited `afl-fuzz` to ≤1 MB inputs but allowed unlimited length inputs for all other fuzzers.
ACKs for top commit:
MarcoFalke:
ACK 6a239e72eb The maximum data size should be a runtime option, not a compile time hardcoded value.
Tree-SHA512: dad176ae39aa09fe919e057008ab0670b9da72909bfeb8f0e8b9ae93b65514f2e25a1d51be89a32be9122fc412edf49234dfd9a44beb974b25fda387fd7bf174
fad798be76 test: Default --previous-releases to false if dir is empty (MarcoFalke)
faf1c3cc58 test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option (MarcoFalke)
Pull request description:
The "auto-detection" feature is kept in place, but making it an option allows to properly document it. For example, on my machine I get:
```
$ ./test/functional/wallet_disable.py --help | grep previous-releases
--previous-releases Force test of previous releases (default: False)
ACKs for top commit:
Sjors:
re-tACK fad798b
Tree-SHA512: a7377d0d5378be0a50be278d76396cc403583617b5fc43467773eee706df698acf3f4e67651491183b9b43a8e1816b052e4c17b90272b7ec4b6ac134ad811400
ca2a09640f Change SetType to SetInternal and remove m_address_type (Andrew Chow)
89b1ce1140 Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow)
b9073c8f13 rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow)
610030d95c docs: Add release notes for descriptor wallets (Andrew Chow)
Pull request description:
Some docs and cleanup following #16528.
* Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
* Adds a warning to `createwallet` that descriptor wallets are experimental.
* Removed unused `SetCrypted` as suggestioned: https://github.com/bitcoin/bitcoin/pull/16528#discussion_r415300916
* Removed `m_address_type` as mentioned in https://github.com/bitcoin/bitcoin/pull/18782#issuecomment-620167077
ACKs for top commit:
Sjors:
tACK ca2a09640f
instagibbs:
utACK ca2a09640f
meshcollider:
utACK ca2a09640f
Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a
1ed52fbb4d Remove IBD check in sethdseed (Andrew Chow)
b1810a145a Test that keys from inactive seeds are generated (Andrew Chow)
c93082ece4 Generate new keys for inactive seeds after marking used (Andrew Chow)
45f2f6a0e8 Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow)
b59b4504ab have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow)
Pull request description:
Largely implements the suggestion from https://github.com/bitcoin/bitcoin/pull/17484#issuecomment-560845316.
After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially.
The indexes and internal-ness of a key is gotten by checking it's key origin data.
Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed.
A test case for this is added as well which fails on master.
ACKs for top commit:
ryanofsky:
Code review ACK 1ed52fbb4d. Changes since last review: various commit message, code comment, log message, error checking improvements, and fix for topping up inactive seeds if wallet isn't reloaded after calling sethdseed and test for this
ariard:
Code Review ACK 1ed52fb
jonatack:
ACK 1ed52fbb4d thanks for addressing the previous review feedback; would be happy to see the new review questions answered and feedback addressed and re-ack.
Tree-SHA512: e658ae0e1dab94be55d2b62cdda506c94815e73a6881533fd30d41cc77477f82fee2095144957a3a1df0c129e256bdd7b7abe3737d515f393610446cae4edf1c
651f1d816f [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069604 [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f
amitiuttarwar:
ACK 651f1d816f🎉
MarcoFalke:
Review ACK 651f1d816f
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
d67055e00d Upgrade or rewrite encrypted key checksums (Andrew Chow)
c9a9ddb414 Set fDecryptionThoroughlyChecked based on whether crypted key checksums are valid (Andrew Chow)
a8334f7ac3 Read and write a checksum for encrypted keys (Andrew Chow)
Pull request description:
Adds a checksum to the encrypted key record in the wallet database so that encrypted keys can be checked for corruption on wallet loading, in the same way that unencrypted keys are. This allows for us to skip the full decryption of keys upon the first unlocking of the wallet in that session as any key corruption will have already been detected. The checksum is just the double SHA256 of the encrypted key and it is appended to the record after the encrypted key itself.
This is backwards compatible as old wallets will be able to read the encrypted key and ignore that there is more data in the stream. Additionally, old wallets will be upgraded upon their first unlocking (so that key decryption is checked before we commit to a checksum of the encrypted key) and a wallet flag set indicating that. The presence of the wallet flag lets us skip the full decryption as if `fDecryptionThoroughlyChecked` were true.
This does mean that the first time an old wallet is unlocked in a new version will take much longer, but subsequent unlocks will be instantaneous. Furthermore, corruption will be detected upon loading rather than on trying to send so wallet corruption will be detected sooner.
Fixes#12423
ACKs for top commit:
laanwj:
code review ACK d67055e00d
jonatack:
Code review ACK d67055e00d
meshcollider:
Code review ACK d67055e00d
Tree-SHA512: d5c1c10cfcb5db9e10dcf2326423565a9f499290b81f3155ec72254ed5bd7491e2ff5c50e98590eb07842c20d7797b4efa1c3475bae64971d500aad3b4e711d4
0187d4c118 [indexes] Add compact block filter headers cache (John Newbery)
Pull request description:
Cache block filter headers at heights of multiples of 1000 in memory.
Block filter headers at height 1000x are checkpointed, and will be the most frequently requested. Cache them in memory to avoid costly disk reads.
ACKs for top commit:
jkczyz:
ACK 0187d4c118
theStack:
ACK 0187d4c118🎉
fjahr:
re-utACK 0187d4c118
laanwj:
code review ACK 0187d4c118
ariard:
Code Review ACK 0187d4c.
Tree-SHA512: 2075ae36901ebcdc4a217eae5203ebc8582181a0831fb7a53a119f031c46bca960a610a38a3d0636a9a405f713efcf4200c85f10c8559fd80139036d89473c56
fa8bbb1368 net: Use C++11 member initialization in protocol (MarcoFalke)
Pull request description:
This change removes `Init` from the constructors and instead uses C++11 member initialization. This removes a bunch of boilerplate, makes the code easier to read. Also, C++11 member initialization avoids accidental uninitialized members.
ACKs for top commit:
laanwj:
ACK fa8bbb1368
Tree-SHA512: f89f6c2fe1bbfccd92acd72c0129d43e464339ed17e95384a81ed33a1a4257dba7ecc1534c6fc8c4668f0d9ade7ba0807b57066c6c763c1b72f74fc51f40907a
4444dbf4d5 gui: Remove un-actionable TODO (MarcoFalke)
Pull request description:
With encryption turned on by default for all wallets in consideration (#18889), I believe that wallet decryption will not be implemented ever or at least any time soon. So remove that TODO comment for now. If deemed important, a brainstorming issue can be opened instead.
Also remove some TODOs in the RPC console, which I don't understand. Maybe the gui was meant to show the debug log interactively? In any case, if deemed important, this should be filed as a brainstorming feature request, so that trade-offs of different solutions can be discussed.
ACKs for top commit:
laanwj:
Thanks. ACK 4444dbf4d5
achow101:
ACK 4444dbf4d5
Tree-SHA512: f7ddb37a14178f575da5409ea1c34e34bde37d79b2b56eaaf606a069e2b91c9d7b734529f5c68664b2fa5aa831117c8d19cce823743671cd6c31b81d68b8c70c
0ea5d70b47 Updated comment for the condition where a transaction relay is denied (glowang)
be01449cc8 Add test for param interaction b/w -blocksonly and -whitelistforcerelay (glowang)
Pull request description:
Related to: #18428
When -blocksonly is turned on, a node would still relay transactions from whitelisted peers. This funcitonality has not been tested.
ACKs for top commit:
MarcoFalke:
ACK 0ea5d70b47
Tree-SHA512: 4e99c88281cb518cc67f5f3be7171a7b413933047b5d24a04bb3ff2210a82e914d69079f64cd5bac9206ec435e21a622c8e69cedbc2ccb39d2328ac5c01668e5
b3f7f375ef refactor: Remove g_rpc_node global (Russell Yanofsky)
ccb5059ee8 scripted-diff: Remove g_rpc_node references (Russell Yanofsky)
6fca33b2ed refactor: Pass NodeContext to RPC and REST methods through util::Ref (Russell Yanofsky)
691c817b34 Add util::Ref class as temporary alternative for c++17 std::any (Russell Yanofsky)
Pull request description:
This PR removes the `g_rpc_node` global, to get same benefits we see removing other globals and make RPC code more testable, modular, and reusable.
This uses a hybrid of the approaches suggested in #17548. Instead of using `std::any`, which isn't available in c++11, or `void*`, which isn't type safe, it uses a small new `util::Ref` helper class, which acts like a simplified `std::any` that only holds references, not values.
Motivation for writing this was to provide an simpler alternative to #18647 by Harris Brakmić (brakmic) which avoids some shortcomings of that PR (https://github.com/bitcoin/bitcoin/pull/18647#issuecomment-617878826)
ACKs for top commit:
MarcoFalke:
re-ACK b3f7f375ef, only change is adding back const and more tests 🚾
ajtowns:
ACK b3f7f375ef
Tree-SHA512: 56292268a001bdbe34d641db1180c215351503966ff451e55cc96c9137f1d262225d7d7733de9c9da7ce7d7a4b34213a98c2476266b58c89dbbb0f3cb5aa5d70
to allow passing rpcwallet independently from the -rpcwallet user option, and to
move the logic to the top-level layer where most of the other option args are
handled.
to ConnectAndCallRPC() to be callable for individual connections.
This is needed for RPCs that need to be called and handled sequentially, rather
than alone or in a batch.
For example, when fetching the balances for each loaded wallet, -getinfo will
call RPC listwallets, and then, depending on the result, RPC getbalances.
It may be somewhat helpful to review this commit with `git show -w`.
e2bab2aa16 multiprocess: add multiprocess travis configuration (Russell Yanofsky)
603fd6a2e7 depends: add MULTIPROCESS depends option (Russell Yanofsky)
5d1377b52b build: multiprocess autotools changes (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR consists of build changes only. It adds an `--enable-multiprocess` autoconf option (off by default and marked experimental), that builds new `bitcoin-node` and `bitcoin-gui` binaries. These currently function the same as existing `bitcoind` and `bitcoin-qt` binaries, but are extended in #10102 with IPC features to execute node, wallet, and gui functions in separate processes.
In addition to adding the `--enable-multiprocess` config flag, it also adds a depends package and autoconf rules to build with the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library, and it adds new travis configuration to exercise the build code and run functional tests with the new binaries.
The changes in this PR were originally part of #10102 but were moved into #16367 to be able to develop and review the multiprocess build changes independently of the code changes. #16367 was briefly merged and then reverted in #18588. Only change since #16367 has been dropping the `native_boost.mk` depends package which was pointed out to be no longer necessary in https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-596484337 and https://github.com/bitcoin/bitcoin/pull/18588#pullrequestreview-391765649
ACKs for top commit:
practicalswift:
ACK e2bab2aa16
Sjors:
tACK e2bab2aa16 on macOS 10.15.4
hebasto:
ACK e2bab2aa16, tested on Linux Mint 19.3 (x86_64):
Tree-SHA512: b5a76eab5abf63d9d8b6d628cbdff4cc1888eef15cafa0a5d56369e2f9d02595fed623f4b74b2cf2830c42c05a774f0943e700f9c768a82d9d348cad199e135c
f852761aec guix: Add clarifying documentation for V env var (Carl Dong)
85f4a4b082 guix: Make V=1 more powerful for debugging (Carl Dong)
Pull request description:
```
- Print commands in both unexpanded and expanded forms
- Set VERBOSE=1 for CMake
```
Ping MarcoFalke hopefully you use `V=1` already for the Guix builds on DrahtBot?
ACKs for top commit:
fanquake:
ACK f852761aec. Ran a Windows Guix build and compared the output from master and this PR when using `V=1`. i.e `HOSTS=x86_64-w64-mingw32 PATH="/root/.config/guix/current/bin${PATH:+:}$PATH" V=1 ./contrib/guix/guix-build.sh`.
Tree-SHA512: 8bc466fa7b869618bbd5a0a91c6b23d4785009289f8dfb93b0349317463a9ab9ece128c72436e02a0819722a63e703100aed15807867a716fda891292fcb9d9d
4a614ff88a test: explicit imports from test_framework.messages in p2p_invalid_messages.py (Sebastian Falbesoner)
b35e1d2471 test: add inventory type constant MSG_CMPCT_BLOCK (Sebastian Falbesoner)
eeaaa58d2c test: replace inv type magic numbers by constants (Sebastian Falbesoner)
Pull request description:
Many functional tests still use magic numbers for inventory types, either passed to the `CInv` constructor or for comparing the `type` member of `CInv`. This PR replaces all of those by constants in the module `test_framework.messages` that have been introduced in commit c32cf9f62285b5cd18a5064aee91f0802f0f87a8: `MSG_TX` (1) or `MSG_BLOCK` (2).
It also introduces a new constant `MSG_CMPCT_BLOCK` (naming as in `src/protocol.h`) and uses it to replace the remaining magic numbers.
The occurences of the magic numbers were identified through `grep`ing for `CInv(` and `type ==`. The idea was first to create a scripted-diff, but since also adding missing `import`s is needed, this would be non-trivial. Besides, also some unneeded comments like `# 2 == "Block"` could be removed.
ACKs for top commit:
gzhao408:
ACK [`4a614ff`](4a614ff88a)
Tree-SHA512: 4ba4fdef9f3eef7fd5ac72cb03ca3524863d1ae292161c550424a4c1047283fa2d2e7e03017d1fbae3652b3cb14f08b8d4b368403f3f209993aef3f2e2b22784
f9ee0f37c2 Add comments to CustomUintFormatter (Pieter Wuille)
4eb5643e35 Convert everything except wallet/qt to new serialization (Pieter Wuille)
2b1f85e8c5 Convert blockencodings_tests to new serialization (Pieter Wuille)
73747afbbe Convert merkleblock to new serialization (Pieter Wuille)
d06fedd1bc Add SER_READ and SER_WRITE for read/write-dependent statements (Russell Yanofsky)
6f9a1e5ad0 Extend CustomUintFormatter to support enums (Russell Yanofsky)
769ee5fa00 Merge BigEndian functionality into CustomUintFormatter (Pieter Wuille)
Pull request description:
The next step of changes from #10785.
This:
* Adds support for enum serialization to `CustomUintFormatter`, used in `CAddress` for service flags.
* Merges `BigEndian` into `CustomUintFormatter`, used in `CNetAddr` for port numbers.
* Converts everything (except wallet and gui) to use the new serialization framework.
ACKs for top commit:
MarcoFalke:
re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter 📂
ryanofsky:
Code review ACK f9ee0f37c2. Just new commit adding comment since last review
jonatack:
Code review re-ACK f9ee0f37c2 only change since last review is an additional commit adding Doxygen documentation for `CustomUintFormatter`.
Tree-SHA512: e7a0a36afae592d5a4ff8c81ae04d858ac409388e361f2bc197d9a78abca45134218497ab2dfd6d031e0cce0ca586cf857077b7c6ce17fccf67e2d367c1b6cd4
fabea6d404 net: Run clang-format on protocol.h (MarcoFalke)
facdeea2b2 net: Remove un-actionable TODO (MarcoFalke)
Pull request description:
The first commit removes a TODO that is infeasible to solve. Currently, most (de)serializable classes in Bitcoin Core have public members. For example `CMessageHeader`, `FlatFilePos`, `CBlock`, `CTransaction`, `CCoin`, ...
So either this TODO comment should apply to all classes or to none. Fix that discrepancy by removing it from the source code for now. If deemed important, the TODO can be discussed in a brainstorming issue later.
Also run clang format on the header file in a new commit. Happy to drop this commit if it is too controversial, but I think it is trivial to review and makes the workflow of developers using clang-format-diff easier.
ACKs for top commit:
practicalswift:
ACK fabea6d404
naumenkogs:
ACK fabea6d. Not sure why that TODO was there in the first place, but Marco's justification seems correct.
hebasto:
ACK fabea6d404, agree with both changes: removing TODO and applying the `clang-format-diff.py`.
Tree-SHA512: b79ae07be27e5a40fc9f411a5e9ae91aecb2fdedbcbf74699614a1004f4ef816bf396903ec6c06eb1395fd83a2047620c7583acbaadfb8c4e613319a63062c3c
faf45d1f1f http: Avoid crash when g_thread_http was never started (MarcoFalke)
fa12a37b27 test: Replace inline-comments with logs, pep8 formatting (MarcoFalke)
fa83b39ff3 init: Remove confusing and redundant InitError (MarcoFalke)
Pull request description:
Avoid a crash during shutdown when the init sequence failed for some reason
ACKs for top commit:
promag:
Tested ACK faf45d1f1f.
ryanofsky:
Code review ACK faf45d1f1f. Thanks for updates, this is much easier to parse for me now. Since previous reviews: split out and reverted some cleanups & replaced chmod with mkdir in test
hebasto:
ACK faf45d1f1f, tested on Linux Mint 19.3 with the following patch:
Tree-SHA512: 59632bf01c999e65c724e2728ac103250ccd8b0b16fac19d3a2a82639ab73e4f2efb86c78e63c588a5954625d8d0cf9545e2a7e070e6e15d2a54beeb50e00b61
d3a56be77a Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" (Russell Yanofsky)
bf0a510981 gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications (Russell Yanofsky)
2bc9b92ed8 Cancel wallet balance timer when shutdown requested (Russell Yanofsky)
83f69fab3a Switch transaction table to use wallet height not node height (Russell Yanofsky)
Pull request description:
Main commit `gui: Avoid wallet tryGetBalances calls` is one-line change to `WalletModel::pollBalanceChanged` that returns early if there hasn't been a new `TransactionChanged` or `BlockTip` notification since the previous poll call. This is the same behavior that was implemented in #18160, now implemented in a simpler way.
The other commits are a straight revert of #18160, and two tweaks to avoid relying on `WalletModel::m_client_model` lifetime which were causing travis failures with earlier versions of this PR.
Motivation for this change is to be able to revert #18160 and cut down on unnecessary cross-process calls that happen when #18160 is combined with #10102
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
jonasschnelli:
utACK d3a56be77a
Tree-SHA512: 3cd31ca515e77c3bd7160d3f1ea0dce5050d4038b2aa441b6f66b8599bd413d81ca5542a197806e773d6092dd1d26830932b1cecbc95298b1f1ab41099e2f12f
e8a8cff07c build: enforce minimum required Windows version (7) (fanquake)
Pull request description:
Instruct the linker to set the major & minor subsystem versions in the PE
header to 6 & 1 (NT 6.1 which corresponds to Windows 7). Similar to
the behaviour on macOS, the binary will now refuse to run on
unsupported versions of Windows, which, for us, is XP & Vista.
ACKs for top commit:
laanwj:
ACK e8a8cff07c
Tree-SHA512: 2f7c6443b79b1c6b995e337452aa177e95b0a9c48e47bcf1893aad6fd598e45940ab8eaa5ee1c5d994a521239b4e1b55a55bb3e8ffe367e1349db2a46892a6d4
- mempool entry 'unbroadcast' field changes when tx passes initial broadcast (receive getdata),
so anytime you compare mempool entries as a whole, you must wait for all broadcasts to complete
('unbroadcast' = False) otherwise the state may change in between calls
- update P2PTxInvStore to send msg_getdata for invs and add functionality to wait for a list
of txids to complete initial broadcast
- make mempool_packages.py wait because it compares entries using getrawmempool and
getmempoolentry
- before reattempting broadcast for unbroadcast txns, check they are in mempool and remove if not
- this protects from memory leaks and network spam just in case unbroadcast set (incorrectly) has extra txns
- check that tx is in mempool before adding to unbroadcast set to try to prevent this from happening
- expose info about number of txns in unbroadcast set and whether a mempool entry's tx has passed initial broadcast
- makes rpcs more informative and allows for more explicit testing, eg tracking if tx is in unbroadcast set
before and after originating node connects to peers (adds this in mempool_unbroadcast.py)
- adds mempool method IsUnbroadcastTx to query for tx inclusion in mempool's unbroadcast set
The "A fatal internal error occurred, see debug.log for details" is
redundant because init.cpp will already show an InitError with a better
error message as well as the hint to check the debug.log
