Optimize Expand by having BIP32PubkeyProvider also cache the parent
(or only) xpub within itself. Since Expand does not provide a read
cache, it is useful to internally cache this xpub to avoid re-deriving
the same xpub.
Also adds tests for this:
For ranged descriptors with unhardened derivation, we expect to
find parent keys in the cache but no child keys.
For descriptors containing an xpub but do not have unhardened derivation
(i.e. hardened derivation or single xpub with or without derivation),
we expect to find all of the keys in the cache, and the same
number of keys in the cache as in the SigningProvider.
For everything else (no xpub), nothing should be cached at all.
Have Expand, ExpandFromCache, and ExpandHelper take additional DescriptorCache
parameters. These are then passed into PubkeyProvider::GetPubKey which
also takes them as arguments.
Reading and writing to the cache is pushed down into GetPubKey. The old cache where
pubkeys are serialized to a vector is completely removed and instead xpubs are being
cached in DescriptorCache.
79facb11e9 wallet: use constant CWallets in rpcwallet.cpp (Karl-Johan Alm)
d9b0ebc1da wallet: make ReserveDestination pwallet ivar const (Karl-Johan Alm)
57c569e4d9 wallet: make BackupWallet() const (Karl-Johan Alm)
df3a818d2a wallet: make getters const (Karl-Johan Alm)
227b9dd2d6 wallet/spkm: make GetOldestKeyPoolTime() const (Karl-Johan Alm)
22d329ad0e wallet: use constant CWallets in rpcdump.cpp (Karl-Johan Alm)
7b3587b29d wallet/db: make IsDummy() const (Karl-Johan Alm)
d366795d18 wallet/db: make Backup() const (Karl-Johan Alm)
8cd0b86340 wallet: make CanGetAddresses() const (Karl-Johan Alm)
037fa770eb wallet: make KeypoolCountExternalKeys() const (Karl-Johan Alm)
ddc93557ad wallet: make CanGenerateKeys() const (Karl-Johan Alm)
dc2d0650fd make BlockUntilSyncedToCurrentChain() const (Karl-Johan Alm)
Pull request description:
A lot of places refer to `CWallet*`'s as `CWallet * const`, which translates to *"an immutable pointer to a mutable `CWallet` instance"*; this is
1. often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and
2. confusing, as it tends to suggest that this is a proper way to refer to a constant `CWallet` instance.
This PR changes references to wallets to `const CWallet* const` whenever immutability is expected. This should result in no behavioral changes at all, and improved compile-time error checking.
Note from irc:
> <sipa> sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn't change
> <sipa> though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move)
> <sipa> CWallet objects aren't copied or moved though
ACKs for top commit:
laanwj:
ACK 79facb11e9
Empact:
ACK 79facb11e9
promag:
ACK 79facb11e9.
fjahr:
ACK 79facb11e9
Tree-SHA512: 80a80c1a52f0f788d0ccb268b53bc0f46c796643a3c5a22b55bbbde4ffa6c7e347784e5e53b1e488a3b4e14399e31d5be9417ad5b6319c74a462609e9b1a98e8
70a6b529f3 lint-cppcheck: Remove -DHAVE_WORKING_BOOST_SLEEP_FOR (Anthony Towns)
294937b39d scheduler_tests: re-enable mockforward test (Anthony Towns)
cea19f6859 Drop unused reverselock.h (Anthony Towns)
d0ebd93270 scheduler: switch from boost to std (Anthony Towns)
b9c4260127 sync.h: add REVERSE_LOCK (Anthony Towns)
306f71b4eb scheduler: don't rely on boost interrupt on shutdown (Anthony Towns)
Pull request description:
Replacing boost functionality with C++11 stuff.
Motivated by #18227, but should stand alone. Changing from `boost::condition_var` to `std::condition_var` means `threadGroup.interrupt_all` isn't enough to interrupt `serviceQueue` anymore, so that means calling `stop()` before `join_all()` is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from `g_lockstack` which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour.
Fixes#16027, Fixes#14200, Fixes#18227
ACKs for top commit:
laanwj:
ACK 70a6b529f3
Tree-SHA512: d1da13adeabcf9186d114e2dad9a4fdbe2e440f7afbccde0c13dfbaf464efcd850b69d3371c5bf8b179d7ceb9d81f4af3cc22960b90834e41eaaf6d52ef7d331
Changes from boost::chrono to std::chrono, boost::condition_var to
std::condition_var, boost::mutex to sync.h Mutex, and reverselock.h to
sync.h REVERSE_LOCK. Also adds threadsafety annotations to CScheduler
members.
Calling interrupt_all() will immediately stop the scheduler, so it's
safe to invoke stop() beforehand, and this removes the reliance on boost
to interrupt serviceQueue().
fae86c38bc util: Remove unused MilliSleep (MarcoFalke)
fa9af06d91 scripted-diff: Replace MilliSleep with UninterruptibleSleep (MarcoFalke)
fa4620be78 util: Add UnintrruptibleSleep (MarcoFalke)
Pull request description:
We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in `std::thread`
ACKs for top commit:
ajtowns:
ACK fae86c38bc quick code review
practicalswift:
ACK fae86c38bc -- patch looks correct
sipa:
Concept and code review ACK fae86c38bc
fanquake:
ACK fae86c38bc - note that an instance of `DHAVE_WORKING_BOOST_SLEEP_FOR` was missed in the [linter](https://github.com/bitcoin/bitcoin/blob/master/test/lint/extended-lint-cppcheck.sh#L69), but that can be cleaned up later.
Tree-SHA512: 7c0f8eb197664b9f7d9fe6c472c77d384f11c797c913afc31de4b532e3b4fd9ea6dd174f92062ff9d1ec39b25e0900ca7c597435add87f0f2477d9557204848c
9ff41f6419 tests: Add float to FUZZERS_MISSING_CORPORA (temporarily) (practicalswift)
8f6fb0a85a tests: Add serialization/deserialization fuzzing for integral types (practicalswift)
3c82b92d2e tests: Add fuzzing harness for functions taking floating-point types as input (practicalswift)
c2bd588860 Add missing includes (practicalswift)
Pull request description:
Add simple fuzzing harness for functions with floating-point parameters (such as `ser_double_to_uint64(double)`, etc.).
Add serialization/deserialization fuzzing for integral types.
Add missing includes.
To test this PR:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/float
…
```
Top commit has no ACKs.
Tree-SHA512: 9b5a0c4838ad18d715c7398e557d2a6d0fcc03aa842f76d7a8ed716170a28f17f249eaede4256998aa3417afe2935e0ffdfaa883727d71ae2d2d18a41ced24b5
7e9c7113af compressor: Make the domain of CompressAmount(...) explicit (practicalswift)
4a7fd7a712 tests: Add amount compression/decompression fuzzing to existing fuzzing harness: test compression round-trip (practicalswift)
Pull request description:
Small fuzzing improvement:
Add amount compression/decompression fuzzing to existing fuzzing harness: test compression round-trip (`DecompressAmount(CompressAmount(…))`).
Make the domain of `CompressAmount(…)` explicit.
Amount compression primer:
```
Compact serialization for amounts
Special serializer/deserializer for amount values. It is optimized for
values which have few non-zero digits in decimal representation. Most
amounts currently in the txout set take only 1 or 2 bytes to
represent.
```
**How to test this PR**
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/integer
…
```
Top commit has no ACKs.
Tree-SHA512: 0f7c05b97012ccd5cd05a96c209e6b4d7d2fa73138bac9615cf531baa3f614f9003e29a198015bcc083af9f5bdc752bb52615b82c5df3c519b1a064bd4fc6664
470e2ac602 tests: Avoid hitting some known minor tinyformat issues when fuzzing strprintf(...) (practicalswift)
Pull request description:
Avoid hitting some known minor tinyformat issues when fuzzing `strprintf(...)`. These can be removed when the issues have been resolved upstreams :)
Note to reviewers: The `%c` and `%*` issues are also present for `%<some junk>c` and `%<some junk>*`. That is why simply matching on `"%c"` or `"%*"` is not enough. Note that the intentionally trivial skipping logic overshoots somewhat (`c[…]%` is filtered in addition to `%[…]c`).
Top commit has no ACKs.
Tree-SHA512: 2b002981e8b3f2ee021c3013f1260654ac7e158699313849c9e9660462bb8cd521544935799bb8daa74925959dc04d63440e647495e0b008cfe1b8a8b2202d40
353f376277 Convert blockencodings.h to new serialization framework (Pieter Wuille)
e574fff53e Add CustomUintFormatter (Pieter Wuille)
10633398f2 Add DifferenceFormatter (Russell Yanofsky)
56dd9f04c7 Make VectorFormatter support stateful formatters (Russell Yanofsky)
3ca574cef0 Convert CCompactSize to proper formatter (Pieter Wuille)
Pull request description:
This is probably the most involved change in the sequence of changes extracted from #10785.
In order to implement the differential encoding of BIP152, this change changes `VectorFormatter` to permit a stateful sub-formatter, which is then used by `DifferenceFormatter`. A `CustomUintFormatter` is added as well to do the 48-bit serialization of short ids.
ACKs for top commit:
laanwj:
ACK 353f376277, nice change
ryanofsky:
Code review ACK 353f376277. Only changes since last review are suggested assert change and MASK->MAX rename
Tree-SHA512: 976618991a8be62ba0738725b7cfa166a56cde998ebf1031ba6f28557032f1577b666ac7ae25cd498c0e1e740108c3c56a342620b724df41d6cc9d8bdafac037
1891245e73 refactor: Cast ping values to double before output (Ben Woosley)
7a810b1d7a refactor: Convert ping wait time from double to int64_t (Ben Woosley)
e6fc63ec7e refactor: Convert min ping time from double to int64_t (Ben Woosley)
b054c46977 refactor: Convert ping time from double to int64_t (Ben Woosley)
Pull request description:
Alternative to #18252, see motivation there.
This changes `CNodeStats` to handle ping timestamps as their original incoming usec `int64_t` values until the time they need to be displayed.
ACKs for top commit:
vasild:
ACK 1891245
practicalswift:
ACK 1891245e73 -- patch looks correct
promag:
ACK 1891245e73, added cast to double and also braces.
Tree-SHA512: 7cfcba941d9751b522b8c512c25da493338b444637bd0bb711b152d7d86b431ca0968956be3c844ee9dbfea25edab44a0de2afa44f2c9c0bf5b8df53eba66272
2455aa5d7f [rpc] changed MineBlocksOnDemand to IsMockableChain (Gloria Zhao)
Pull request description:
Change: Update the if statement in `setmocktime` to use `IsMockableChain` chainparams function (aka `m_is_mockable_chain`) instead of `MineBlocksOnDemand`
Rationale: It's a more appropriate check for whether or not chain is in RegTest, as [discussed](https://github.com/bitcoin/bitcoin/pull/18037#discussion_r376509388) in #18037
ACKs for top commit:
MarcoFalke:
ACK 2455aa5d7f🙇
jonatack:
ACK 2455aa5d7f
Tree-SHA512: 1d8c8b7ff0b3c1bcbf5755194969b6664fe05a35003375ad08d18e34bcefd2df4f64d0e60078a10bbef3c8f469a9b9d07db467089b55c14cf532304bc965bffc
and update feature_asmap.py and test_runner.py
This commit moves the asmap init.cpp code from the end of "Step 12: start node"
to "Step 6: network initialization" to provide feedback on passing an -asmap
config arg much more quickly. This change speeds up the feature_asmap.py
functional test file from 60 to 5 seconds by accelerating the 2 tests that use
`assert_start_raises_init_error`.
Credit to Wladimir J. van der Laan for the suggestion.
- move asmap #includes to sorted positions in addrman and init (move-only)
- remove redundant quotes in asmap InitError, update test
- remove full stops from asmap logging to be consistent with debug logging,
update tests
fa6b061fc1 rpc: Auto-format RPCResult (MarcoFalke)
fa7d0503d3 rpc: Move OuterType enum to header (MarcoFalke)
Pull request description:
This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests)
Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/
ACKs for top commit:
Sjors:
Indeed, re-ACK fa6b061fc1
ajtowns:
ACK fa6b061fc1 -- skimmed code changes and differences to rpc help output
Tree-SHA512: 5b510b3aa0b7c7b9189a48c77593159409069f939145b9a00c5478e894cf65f994d44d633eb7bb7dbea40ee820645a2930976c24772379d96929002b120efa28
aff2748f8a httpserver: use own HTTP status codes (Filip Gospodinov)
Pull request description:
Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.
Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.
ACKs for top commit:
practicalswift:
ACK aff2748f8a -- patch looks correct
laanwj:
ACK aff2748f8a
Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
1ef28b4f7c Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)
Pull request description:
Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220
Sjors documenting the issue:
```
A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))
{
"inputs": [
{
"has_utxo": true,
"is_final": false,
"next": "finalizer"
}
],
"estimated_vsize": 141,
"estimated_feerate": 1e-05,
"fee": 1.41e-06,
"next": "signer"
}
I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
```
It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.
Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.
ACKs for top commit:
Sjors:
ACK 1ef28b4f7c, much nicer. Don't forget to document the bug fix.
achow101:
ACK 1ef28b4f7c
Empact:
ACK 1ef28b4f7c
Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
* GetAvoidReuseFlag: simply gets the flag, without modifying the wallet
* ListReceived: helper function to produce lists
* ListTransactions: produces a list of transactions, without modifications; two cases of map [] -> .at() for verified-existing keys
* DescribeWalletAddress: generates a description of a given wallet address without changing the wallet
* The following functions produce a list without making any modifications to the wallet:
* listaddressgroupings
* listreceivedbyaddress
* listreceivedbylabel
* listtransactions
* listsinceblock
* listlockunspent
* listunspent
* listlabels
* getreceivedbyaddress
* getreceivedbylabel
* getaddressesbylabel
* signmessage: uses the wallet to procure a private key for signing, but does no modifications
* getbalance, getunconfirmedbalance: calculates the wallet balance, without any modifications
* gettransaction: procures transaction without any modifications
* backupwallet: makes a backup of the wallet to disk, without changing said wallet
* getwalletinfo: produces info about wallet without any modifications
* signrawtransactionwithwallet: modifies incoming transaction on the fly by signing with private key procured from within wallet; no modifications to wallet
* getaddressinfo: gets information about the given address, with no modifications done to the wallet; one case of [] -> .at() and one ::iterator -> ::const_iterator
* walletprocesspsbt: processes the given PSBT on the fly, without modifying the wallet
* GetWalletAddressesForKey is, as the name implies, immutable; the one change besides the parameter constness is a [] -> .at() change, to a verified-existing key.
* dumpprivkey and dumpwallet are both similarly immutable, for obvious reasons.
CWallet::CanGetAddresses() is used to check whether the wallet has available or is able to produce keys for addresses. It uses the ScriptPubKeyMan::CanGetAddresses(), which in turn uses the const KeypoolCountExternalKeys() method, all which do counting and no modifications.
10efc0487c Templatize ValidationState instead of subclassing (Jeffrey Czyz)
10e85d4adc Remove ValidationState's constructor (Jeffrey Czyz)
0aed17ef28 Refactor FormatStateMessage into ValidationState (Jeffrey Czyz)
Pull request description:
This removes boilerplate code in the subclasses which otherwise only
differ by the result type.
The subclassing was introduced in a27a295.
ACKs for top commit:
MarcoFalke:
ACK 10efc0487c🐱
ajtowns:
ACK 10efc0487c -- looks good to me
jonatack:
ACK 10efc048 code review, build/tests green, nice cleanup
Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42
d36146009f Drop unused mach time headers (Ben Woosley)
Pull request description:
Now that we're no longer special-casing clock usage for MacOS (see #17800), we're
not referencing anything defined in these headers.
Incidentally, this removes our last reference to the `__MACH__` system def. 🎉
ACKs for top commit:
jonasschnelli:
utACK d36146009f
fanquake:
ACK d36146009f - thanks.
Tree-SHA512: 246045b0683a705ad034416e8ace2024e652026a6c0517b6797320e52fc18a6e111ec2e405ca40653bd1d6421bb7755232e8fec22651fff8e448eb7d5646a954
8888461f68 util: Fail to parse empty string in ParseMoney (MarcoFalke)
fab30b61eb util: Remove unused ParseMoney that takes a c_str (MarcoFalke)
Pull request description:
Supplying a fee rate or an amount on the command line as an empty string, which currently parses as `0` seems fragile and confusing. See for example the confusion in #18214.
Fixes#18214
ACKs for top commit:
Empact:
Code Review ACK 8888461f68
achow101:
ACK 8888461f68
instagibbs:
utACK 8888461f68
Tree-SHA512: ac2d6b7fa89fe5809c34d5f49831042032591c34fb3c76908d72fed51e8bced41bf2b41dc1b3be34ee691a40463355649857a7a8f378709d38ae89503feb11c2
16d6113f4f Refactor message transport packaging (Jonas Schnelli)
Pull request description:
This PR factors out transport packaging logic from `CConnman::PushMessage()`.
It's similar to #16202 (where we refactor deserialization).
This allows implementing a new message transport protocol like BIP324.
ACKs for top commit:
dongcarl:
ACK 16d6113f4f FWIW
ariard:
Code review ACK 16d6113
elichai:
semiACK 16d6113f4f ran functional+unit tests.
MarcoFalke:
ACK 16d6113f4f🙎
Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1
dc9305b616 random: don't special case clock usage on macOS (fanquake)
Pull request description:
`clock_gettime()`, `CLOCK_MONOTONIC` and `CLOCK_REALTIME` are all available for use on
macOS (now that we require macOS >=10.12 and build against 10.14). Use them rather than the [deprecated](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/KernelProgramming/Mach/Mach.html) `mach_timespec_t` time API.
I mentioned the possibility for this change [in #17270](https://github.com/bitcoin/bitcoin/pull/17270#discussion_r346090606).
[master](1dbf3350c6):
```bash
2019-12-23T20:49:43Z Feeding 216 bytes of dynamic environment data into RNG
2019-12-23T20:50:43Z Feeding 216 bytes of dynamic environment data into RNG
```
This PR:
```bash
2019-12-23T20:32:41Z Feeding 232 bytes of dynamic environment data into RNG
2019-12-23T20:33:42Z Feeding 232 bytes of dynamic environment data into RNG
```
~~Depends on #16392.~~ Merged.
ACKs for top commit:
laanwj:
ACK dc9305b616
Tree-SHA512: 18c2f336ea628f9cf7339b817381d230a18893fd9c0351bf99a39ca6f45c5b0a20af9d599d48d6c09515627d5edafa91337c17f9f790264251d2cdcb3763bbd5
7bf4ce4f64 refactor: test/bench: dedup SetupDummyInputs() (Sebastian Falbesoner)
Pull request description:
The only difference between `SetupDummyInputs()` in `test/transaction_tests.cpp` and the one in `bench/ccoins_caching.cpp` was the nValue amounts of the outputs, so we allow to pass those in an extra (fixed-size) array parameter.
ACKs for top commit:
MarcoFalke:
re-ACK 7bf4ce4f64, only change is schuffling includes 🚶
Empact:
ACK 7bf4ce4f64
Tree-SHA512: e13643b2470f6b6ab429da0c0a8eebd4cb41e2ff2e421ef36f85fa4847bf4ea8aab88d59a01e94cac4c4eb85edb561463f02215b174c50b573ac6bbcc2bf98a3
The function IsStandardTx() returns rejection reason "scriptsig-not-pushonly"
if the transaction has at least one input for which the scriptSig consists of
any other ops than just PUSHs.
The only difference between SetupDummyInputs() in test/transaction_tests.cpp
and the one in bench/ccoins_caching.cpp was the nValue amounts of the outputs,
so we allow to pass those in an extra (fixed-size) array parameter.
c72a11a1a0 test: Add cost_of_change parameter assertions to bnb_search_test (Yancy Ribbens)
Pull request description:
If the `cost_of_change` variable is removed from the method body `SelectCoinsBnB`, there are currently no failing unit tests. This PR adds assertions about the behavior of the `cost_of_change`: If the cost of creating a change output is greater than what's leftover, then consume the output and create no change, otherwise, don't consume the output (no match found).
ACKs for top commit:
achow101:
ACK c72a11a1a0
Tree-SHA512: 613aa411df5e2911446e0e8bf3309336faaadf2d3c56e7d125b76454e7c6f9e4f5e8f0910dc6222282628e38cd8a4a7c56bb3d36b564a17f396b9b503ecc64c8
fab2527515 test: Disable mockforward scheduler unit test for now (MarcoFalke)
Pull request description:
This should be a workaround to fix#18174 in the short run and buy us more time to investigate the issue while ci runs are green again 🙏
ACKs for top commit:
fanquake:
ACK fab2527515 - be good to get Travis back.
laanwj:
ACK fab2527515
Tree-SHA512: 027e86b3dfec203a464e5bf528e9933c208c36633c2d4bfcdbc10da1799637a5d6ea0a63af33a4174fb1ad7115df631a4cb838f56e31f4cbd15498e1e9fdf9cc
0653939ac1 Add static_asserts to ser_X_to_Y() methods (Samer Afach)
be94096dfb Fix a violation of C++ standard rules that unions cannot be switched. (Samer Afach)
Pull request description:
Type punning in C++ is not like C. As per the C++ standard, one cannot use unions to convert the bit type. A discussion about this can be found [here](https://stackoverflow.com/questions/25664848/unions-and-type-punning). In C++, a union is supposed to only hold one type at a time. It's intended to be used only as `std::variant`. Switching types is undefined behavior.
In fact, C++20 has a special casting function, called [`bit_cast`](https://en.cppreference.com/w/cpp/numeric/bit_cast) that solved this problem.
Why has it been working so far? Because some compilers tolerate using unions and switching types, like gcc. More information [here](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Type-punning).
One important thing to mention is that performance is generally not affected by that memcpy. Compilers are smart enough to convert that to a memory cast when possible. But we have to do it the right way, otherwise, it's jut undefined behavior that depends on the compiler.
ACKs for top commit:
practicalswift:
ACK 0653939ac1
elichai:
ACK 0653939ac1
laanwj:
Code review ACK 0653939ac1
kristapsk:
ACK 0653939ac1
Tree-SHA512: f6e89de39fc964750429139bab6b5a1346f7060334b7afa020e315bdad8f8c195bce2b8a9e343f06e7fff175e2dfb1cdabfcb6fe405bea0febe4962f0cc62557
facb71576c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b346, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
This is needed so that it can be used by RPCResult
Also,
* rename NAMED_ARG to NONE for generalization.
* change RPCArg constructors to initialize the members by moving values
5bad7921d0 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9061 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
e193a84fb2 Refactor message hashing into a utility function (Jeffrey Czyz)
f8f0d9893d Deduplicate the message signing code (Vasil Dimov)
2ce3447eb1 Deduplicate the message verifying code (Vasil Dimov)
Pull request description:
The message signing and verifying logic was replicated in a few places
in the code. Consolidate in a newly introduced `MessageSign()` and
`MessageVerify()` and add unit tests for them.
ACKs for top commit:
Sjors:
re-ACK e193a84fb2
achow101:
ACK e193a84fb2
instagibbs:
utACK e193a84fb2
meshcollider:
utACK e193a84fb2
Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88
12a2f37718 util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t nTime) by checking gmtime_s/gmtime_r return value (practicalswift)
Pull request description:
Avoid potential uninitialized read in `FormatISO8601DateTime(int64_t)` by checking `gmtime_s`/`gmtime_r` return value.
Before this patch `FormatISO8601DateTime(67768036191676800)` resulted in:
```
==5930== Conditional jump or move depends on uninitialised value(s)
==5930== at 0x4F44C0A: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==5930== by 0x4F511A4: std::ostream& std::ostream::_M_insert<long>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==5930== by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358)
==5930== by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:543)
==5930== by 0x402E02: tinyformat::detail::FormatArg::format(std::ostream&, char const*, char const*, int) const (tinyformat.h:528)
==5930== by 0x401B16: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:907)
==5930== by 0x4017AE: tinyformat::vformat(std::ostream&, char const*, tinyformat::FormatList const&) (tinyformat.h:1054)
==5930== by 0x401765: void tinyformat::format<int, int, int, int, int, int>(std::ostream&, char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1064)
==5930== by 0x401656: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1073)
==5930== by 0x4014CC: FormatISO8601DateTime[abi:cxx11](long) (…)
```
The same goes for other very large positive and negative arguments.
Fix by simply checking the `gmtime_s`/`gmtime_r` return value :)
ACKs for top commit:
MarcoFalke:
ACK 12a2f37718
theStack:
re-ACK 12a2f37718
elichai:
re ACK 12a2f37718
Tree-SHA512: 066142670d9bf0944d41fa3f3c702b1a460b5471b93e76a619b1e818ff9bb9c09fe14c4c37e9536a04c99533f7f21d1b08ac141e1b829ff87ee54c80d0e61d48
bca8665d08 scripted-diff: Wallet: Rename incorrectly named *UsedDestination (Luke Dashjr)
Pull request description:
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
ACKs for top commit:
practicalswift:
ACK bca8665d08 -- patch looks correct and rationale makes sense
instagibbs:
ACK bca8665d08, much more meaningful name, thanks
kallewoof:
ACK bca8665d08
Tree-SHA512: ff13d9061ffa748e92eb41ba962c3ec262a43e4b6abd62408b38c6f650395d6ae5851554257d1900fb02767a88d08380d592a27210192ee9abb72d0945976686
faca8eff39 test: Remove incorrect assumptions in validation_flush_tests (MarcoFalke)
fa31eebfe9 test: Tabs to spaces in all tests (MarcoFalke)
Pull request description:
The tests assume standard library internals that may not hold on all supported archs or when the code is instrumented for sanitizer or debug use cases
Fixes#18111
ACKs for top commit:
jamesob:
ACK faca8eff39 pending passing tests
fjahr:
ACK faca8eff39
Tree-SHA512: 60a5ae824bdffb0762f82f67957b31b185385900be5e676fcb12c23d53f5eea734601680c2e3f0bdb8052ce90e7ca1911b1342affb67e43d91a506b111406f41
fac52dafa0 test: Set catch_system_errors=no on boost unit tests (MarcoFalke)
Pull request description:
Closes#16700
Can be tested by adding an `assert(0)` and then running either `make check` or `./src/test/test_bitcoin -t bla_tests --catch_system_errors=no/yes`
ACKs for top commit:
practicalswift:
ACK fac52dafa0
Empact:
Tested ACK fac52dafa0
Tree-SHA512: ec00636951b2c1137aaf43610739d78d16f823f7da76a726d47f93b8b089766fb66b21504b3c5413bcf8b6b5c3db0ad74027d677db24a44487d6d79a6bdee2e0
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
7e80f646b2 Get the OutputType for a descriptor (Andrew Chow)
Pull request description:
Adds a `GetOutputType()` method to get the OutputType of a descriptor. Some descriptors don't have a determinate OutputType, so we actually use an `Optional<OutputType>`. For descriptors with indeterminate OutputType, we return `nullopt`.
`addr()` and `raw()` use OutputTypes as determined by the CTxDestination they have. For simplicity, `ScriptHash` destinations are `LEGACY` even though they could be `P2SH_SEGWIT`.
`combo()`, `pk()`, and `multi()` are `nullopt` as they either don't have an OutputType or they have multiple. `DescriptorImpl` defaults to `nullopt`.
`pkh()` is `LEGACY` as expected
`wpkh()` and `wsh()` are `BECH32` as expected.
`sh()` checks whether the sub-descriptor is `BECH32`. If so, it is `P2SH_SEGWIT`. Otherwise it is `LEGACY`.
The descriptor tests are updated to check the OutputType too.
ACKs for top commit:
fjahr:
ACK 7e80f646b2
meshcollider:
utACK 7e80f646b2
instagibbs:
cursory ACK 7e80f646b2
Sjors:
Code review ACK 7e80f646b2
jonatack:
ACK 7e80f64 code review/build/tests
Tree-SHA512: c5a813447b62e982435e1c948066f8d6c148c9ebffb0a5eb5a9028b173b01d5ead2f076a5ca3f7f37698538baa346f82a977ee48f583d89cb4e5ebd9111b2341
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
7f1475c711 rpc: update validateaddress RPCExamples to bech32 (Sebastian Falbesoner)
Pull request description:
Another small step to get rid of legacy addresses in the RPC help texts and by that encourage the use of bech32 addresses by default. The (invalid) address is the same as in the `getaddressinfo` RPC (see 2ee0cb3330, kudos to jonatack!), I don't think it adds any value to have a different example address per RPC.
ACKs for top commit:
fanquake:
ACK 7f1475c711
MarcoFalke:
ACK 7f1475c711
Tree-SHA512: 2350f61fa942a9053f9f5c860ea446965dc7209c71c81bdb98a859d03ca23b225ad72c9c506e4a55c8d8988823d9cfbe808c1a452a1eeadb70ab186b146dd4ca
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f
meshcollider:
utACK a304a3632f
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
Before, macros defined in `<event2/http.h>` have been used
for some HTTP status codes.
`<event2/http.h>` is included implicitly and the usage
of its status code macros is inconsistent with the majority
HTTP response implementations in this file.
Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is
consistently used for all HTTP response implementations.
8bca30ea17 [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5b52 [lib] add scheduler to node context (Amiti Uttarwar)
930d837542 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e83c6 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f63598ad [util] allow scheduler to be mocked (Amiti Uttarwar)
Pull request description:
This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.
It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.
This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.
ACKs for top commit:
MarcoFalke:
ACK 8bca30ea17, only change is some style fixups 🕓
Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)
Pull request description:
Fixes#13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
fad027fb0c scripted-diff: Add missing spaces in RPCResult, Fix type names (MarcoFalke)
Pull request description:
This makes the rendered diff smaller when the RPCResult is machine generated later on (Previous attempts: #14601 and #14459)
ACKs for top commit:
Sjors:
ACK fad027fb0c
Tree-SHA512: 48afd571b1cd349ca0b29bb444c1c7cda657e07dd96c610d479f931ccd938186aec98e533d0552b5b10afc9a3d7b911359260a49448e8e1106e3647b2c71f3ba
And add unit test for it.
The purpose of using a preamble or "magic" text as part of signing and
verifying a message was not given when the code was repeated in a few
locations. Make a test showing how it is used to prevent inadvertently
signing a transaction.
The logic of signing a message was duplicated in 3 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
src/rpc/misc.cpp
signmessagewithprivkey()
src/wallet/rpcwallet.cpp
signmessage()
Move the logic into
src/util/message.cpp
MessageSign()
and call it from all the 3 places.
The logic of verifying a message was duplicated in 2 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()
src/rpc/misc.cpp
verifymessage()
with the only difference being the result handling. Move the logic into
a dedicated
src/util/message.cpp
MessageVerify()
which returns a set of result codes, call it from the 2 places and just
handle the results differently in the callers.
25bc17fceb refactor: rpc: Remove vector copy from listtransactions (João Barbosa)
Pull request description:
Current approach
- copy accumulated `ret` vector to `arrTmp`
- drop unnecessary elements from `arrTmp`
- reverse `arrTmp`
- clear `ret`
- copy `arrTmp` to the `ret`
New approach
- create a vector from the accumulated `ret` with just the necessary elements already reversed
- copy it to the result
This PR doesn't change behavior.
ACKs for top commit:
ryanofsky:
Code review ACK 25bc17fceb. Just comment and commit message tweaks since last review
Tree-SHA512: 87906561e3accdbdb0f4a8194cbcd76ea53ae53d0ce135b90bc54a5f77e300b14ef08505e7daf1fe52426f135442a743da5a027416a769bd454922357cebe7c0
also contains the following changes:
- rpc: factor out example bech32 address for RPCExamples
- doc: update developer notes wrt RPCExamples addresses
(mention the EXAMPLE_ADDRESS constant as an example for an invalid bech32
address suitable for RPCExamples help documentation)
c9fe61291e gui: Throttle GUI update pace when -reindex (Hennadii Stepanov)
Pull request description:
This is grabbed from #17565.
All **laanwj**'s and **ryanofsky**'s suggestions are implemented.
With this PR, the GUI does not freeze when a user runs:
```
$ ./src/qt/bitcoin-qt -reindex
```
ACKs for top commit:
jonasschnelli:
utACK c9fe61291e
Tree-SHA512: c7be316cb73d3d286bdf8429a960f71777d13a73d059869a64e23ad276499252b561a3a5b9613c4c1ad58cc0de26283c1ec72be745c401f604eaa05f70bf7d64
bf36a3ccc2 gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky)
Pull request description:
Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling.
This bug could cause balances to appear out of date, and was first introduced a0704a8996 (diff-2e3836af182cfb375329c3463ffd91f8L117). Before that commit, there wasn't a problem because cs_main was held during the poll update.
Currently, the problem should be rare. But if 8937d99ce81a27ae5e1012a28323c0e26d89c50b from #17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification.
MarcoFalke also points out that a0704a8996 could lead to GUI hangs as well, because previously the pollBalanceChanged method, which runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call, but after could make blocking LOCK(cs_main) calls, potentially locking up the GUI.
Thanks to John Newbery for finding this bug this while reviewing https://github.com/bitcoin/bitcoin/pull/17954.
ACKs for top commit:
Empact:
utACK bf36a3ccc2
jonasschnelli:
utACK bf36a3c
Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4
5f26855f10 test: Remove ubsan alignment suppressions (Wladimir J. van der Laan)
9d933ef919 prevector: avoid misaligned member accesses (Anthony Towns)
Pull request description:
Ensure prevector data is appropriately aligned. Earlier discussion in #17530.
**Edit laanwj**: In contrast to #17530, it does this without increase in size of any of the coin cache data structures (x86_64, clang)
| Struct | (size,align) before | (size,align) after |
| ------------- | ------------- | ------- |
| Coin | 48, 8 | 48, 8 |
| CCoinsCacheEntry | 56, 8 | 56, 8 |
| CScript | 32, 1 | 32, 8 |
ACKs for top commit:
laanwj:
ACK 5f26855f10
practicalswift:
ACK 5f26855f10
jonatack:
ACK 5f26855f10
Tree-SHA512: 98d112d6856f683d5b212410b73f3071d2994f1efb046a2418a35890aa1cf1aa7c96a960fc2e963fa15241e861093c1ea41951cf5b4b5431f88345eb1dd0a98a
Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts
when the redeem script is present in the mapScripts map without the p2sh script
also having to be added to the mapScripts map. This restores behavior prior to
https://github.com/bitcoin/bitcoin/pull/17261, which I think broke backwards
compatibility with old wallet files by no longer treating addresses created by
`addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261
is because of a workaround added in 4a7e43e846
"Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem
for new `addmultisigaddress` RPC calls without fixing it for multisig addresses
already created in old wallet files.
This change adds a lot of comments and allows reverting commit
4a7e43e846 "Store p2sh scripts in
AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function,
CanProvide() method, and mapScripts map should all be more comprehensible
Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.
This bug could cause balances to appear out of date, and was first introduced
a0704a8996 (r378452145)
Before that commit, there wasn't a problem because cs_main was held during the
poll update.
Currently, the problem should be rare. But if
8937d99ce81a27ae5e1012a28323c0e26d89c50b from #17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.
MarcoFalke <falke.marco@gmail.com> also points out that a0704a8996 could lead
to GUI hangs as well, because previously the pollBalanceChanged method, which
runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call,
but after could make blocking LOCK(cs_main) calls, potentially locking up the
GUI.
Thanks to John Newbery <john@johnnewbery.com> for finding this bug this while
reviewing https://github.com/bitcoin/bitcoin/pull/17954.
4537ba5f21 test: add unit test for non-standard txs with too large tx size (Sebastian Falbesoner)
Pull request description:
Approaches another missing unit test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"tx-size"` if the transaction weight is larger than `MAX_STANDARD_TX_WEIGHT` (=400000 vbytes).
ACKs for top commit:
Empact:
Code Review ACK 4537ba5f21
instagibbs:
ACK 4537ba5f21
Tree-SHA512: ab32e3e47e0b337253aef3da9b7c97d01f4130d00d5860588dfed02114eec3ba49473acc6419448affd63e883fd827bf308716965606eaddee242c4c5a4eb799
3c94b0039d Convert undo.h to new serialization framework (Pieter Wuille)
3cd8ab9d11 Make std::vector and prevector reuse the VectorFormatter logic (Pieter Wuille)
abf8624356 Add custom vector-element formatter (Pieter Wuille)
37d800bea0 Add a constant for the maximum vector allocation (5 Mbyte) (Pieter Wuille)
Pull request description:
The next step of changes from #10785.
This one adds:
* A meta-formatter for vectors, which serializes the vector elements using another formatter
* Switch the undo.h code to the new framework, using the above (where undo entries are serialized as a vector, each of which uses a modified serializer for the UTXOs).
ACKs for top commit:
laanwj:
code review ACK 3c94b0039d
jonatack:
Qualified ACK 3c94b0039d
ryanofsky:
Code review ACK 3c94b0039d. Changes since last review: renaming formatter classes, adding suggested static_assert, and removing temporary in VectorFormatter
Tree-SHA512: 44eebf51a303f6adbbc1ca2b9d043e8ae7fd37e06778e026590892f8d09f8253067862a68ba8ca5d733fd2f8e7c84edd255370f5a4b6560259427a65f94632df
ac57859e53 qt: Fix deprecated QCharRef usage (Hennadii Stepanov)
Pull request description:
From Qt docs:
- [`QKeyEvent::text()`](https://doc.qt.io/qt-5/qkeyevent.html#text):
> Return values when modifier keys such as Shift, Control, Alt, and Meta are pressed differ among platforms and could return an empty string.
- [`QString::operator[]()`](https://doc.qt.io/qt-5/qstring.html#operator-5b-5d):
> **Note:** Before Qt 5.14 it was possible to use this operator to access a character at an out-of-bounds position in the string, and then assign to such a position, causing the string to be automatically resized. Furthermore, assigning a value to the returned `QCharRef` would cause a detach of the string, even if the string has been copied in the meanwhile (and the `QCharRef` kept alive while the copy was taken). These behaviors are deprecated, and will be changed in a future version of Qt.
Since Qt 5.14 this causes a `QCharRef` warning if any modifier key is pressed while the splashscreen is still displayed.
Fix#18080.
Note: Ctrl+Q will also close the spashscreen now.
ACKs for top commit:
jonasschnelli:
utACK ac57859e53
Tree-SHA512: a7e5559410bd05c406007ab0243f458b82d434b0543276ed331254c8d7a6b1aaa54d0b406f799b830859294975004380160f8af04ba403d3bf185d51e6784f54
2af3e16ca9 Qt: pass clientmodel changes from walletframe to walletviews (Jonas Schnelli)
Pull request description:
Fixes#18090
We currently don't pass `clientmodel` changes from the `walletframe` to the `walletviews` leading to possible invalid access during shutdown because all walletviews miss the nullifying of the clientmodel.
TODO: needs investigation if this is should be backported.
ACKs for top commit:
laanwj:
Good catch, code review ACK 2af3e16ca9
Tree-SHA512: f8c0a114f01deac07fb311112d144f3bfc1c1882dd19e8742b372dd597d7a5d59cd0af99fc50494de2334cad98d6701675317474e40fe8820d04c058aeca1b75
677fb8e923 test: Add ubsan surpression for crc32c (Wladimir J. van der Laan)
8e68bb1dde build: Disable msvc warning 4722 for leveldb build (Aaron Clauson)
be23949765 build: MSVC changes for leveldb update (Aaron Clauson)
9ebdf04757 build: CRC32C build system integration (Wladimir J. van der Laan)
402252a808 build: Add LCOV exception for crc32c (Wladimir J. van der Laan)
3a037d0067 test: Add crc32c exception to various linters and generation scripts (Wladimir J. van der Laan)
84ff1b2076 test: Add crc32c to subtree check linter (Wladimir J. van der Laan)
7cf13a5134 doc: Add crc32c subtree to developer notes (Wladimir J. van der Laan)
24d02a9ac0 build: Update build system for new leveldb (Wladimir J. van der Laan)
2e1819311a Squashed 'src/crc32c/' content from commit 224988680f7673cd7c769963d4035cb315aa3388 (Wladimir J. van der Laan)
66480821b3 Squashed 'src/leveldb/' changes from f545dfabff4c2e9836efed094dba99a34fbc6b88..f8ae182c1e5176d12e816fb2217ae33a5472fdd7 (Wladimir J. van der Laan)
Pull request description:
This updates leveldb to currently newest upstream commit 0c40829872:
- CRC32C hardware acceleration is now an external library [crc32c](https://github.com/google/crc32c). This adds acceleration on ARM, and should be faster on x86 because of using prefetch. It also makes it easy to support similar instruction sets on other platforms in the future.
- Thread handling uses C++11, instead of platform specific code.
- Native windows environment was added. No need to maintain our own hacky one, anymore.
- Upstream now builds using CMake. This doesn't mean we need to use that (phew), but internal configuration changed to a a series of checks, instead of OS profiles. This means the blanket error "Cannot build leveldb for $host. Please file a bug report' is removed.
All changes: a53934a3ae...0c40829872
Pretty much all our changes have been subsumed by upstream, so we figured it was cleaner to start over with a new branch from upstream with the still-relevant patches applied: https://github.com/bitcoin-core/leveldb/tree/bitcoin-fork-new
There's quite some testing to be done (see below). See https://github.com/bitcoin-core/leveldb/issues/25 and https://github.com/bitcoin-core/leveldb/pull/26 for more history and context.
TODO:
- [x] Subtree `crc32c`
- [x] Make linters happy about crc32 subtree
- [x] Integrate `crc32c` library into build system
- [x] MSVC build system
ACKs for top commit:
sipa:
ACK 677fb8e923
Tree-SHA512: 37ee92a750e053e924bc4626b12bb3fd81faa9f8c5ebaa343931fee810c45ba05aa6051fdea82535fa351bf2be7297801b98af9469865fc5ead771650a5d6240
19a354b11f Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)
Pull request description:
Give a descriptor from `createmultisig` and `addmultisigaddress`.
Extracted from #16528 with `addmultisgaddress` and tests added.
ACKs for top commit:
Sjors:
tACK 19a354b11f
MarcoFalke:
ACK 19a354b11f
promag:
Code review ACK 19a354b11f.
meshcollider:
utACK 19a354b11f
Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771
Replace the memset/strncpy dance in `CMessageHeader::CMessageHeader`
with explicit code that copies then name and asserts the length.
This removes a warning in g++ 9.1.1 and IMO makes the code more readable
by not relying on strncpy padding and silent truncation behavior.
acf8abc7f3 gui: Fix unintialized WalletView::progressDialog (João Barbosa)
Pull request description:
#17911 shows that it's possible to read the unintialized `progressDialog` in f32564f0a7/src/qt/walletview.cpp (L296-L297).
And the debugger shows
```
(gdb) bt
#0 0x0000555556687c60 in QProgressDialog::wasCanceled() const ()
#1 0x000055555572989f in WalletView::showProgress (this=0x5555577d7a70,
title=..., nProgress=1) at qt/walletview.cpp:322
```
Closes#17911.
ACKs for top commit:
hebasto:
ACK acf8abc7f3, I have reviewed the code and it looks OK, I agree it can be merged.
elichai:
utACK acf8abc7f3
kristapsk:
ACK acf8abc7f3
MarcoFalke:
ACK acf8abc7f3
Tree-SHA512: f5e6d873192d08d1a572e66e17c2e06d1ce27d01aa196b2a7ed591008641295bb02cda8ac90919ff2d2fc778316c2e143f8d36599e0d377779758853dfaf0a31
clock_gettime(), CLOCK_MONOTONIC and CLOCK_REALTIME are all available for use on
macOS (now that we require macOS >=10.12). Use them rather than the deprecated
mach_timespec_t time API.
master:
2019-12-23T20:49:43Z Feeding 216 bytes of dynamic environment data into RNG
2019-12-23T20:50:43Z Feeding 216 bytes of dynamic environment data into RNG
this commit:
2019-12-23T20:32:41Z Feeding 232 bytes of dynamic environment data into RNG
2019-12-23T20:33:42Z Feeding 232 bytes of dynamic environment data into RNG
900d8f6f70 util: Disallow network-qualified command line options (Russell Yanofsky)
Pull request description:
Previously these were allowed but ignored.
This change implements one of the settings simplifications listed in #17508. Change includes release notes.
ACKs for top commit:
laanwj:
ACK 900d8f6f70
Tree-SHA512: ab020a16a86c1e8ec709fbf798d533879d32c565eceeb7eb785c33042c49c6b4d1108c5453d8166e4a2abffc2c8802fbb6d3b895e0ddeefa8f274fd647e3c8ad
e9434ee03e Remove false positive GCC warning (Hennadii Stepanov)
Pull request description:
On master (f05c1ac444) GCC compiler fires a false positive `-Wmaybe-uninitialized`:
```
wallet/wallet.cpp: In static member function ‘static std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::vector<std::__cxx11::basic_string<char> >&, uint64_t)’:
wallet/wallet.cpp:3913:27: warning: ‘*((void*)& time_first_key +8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Optional<int64_t> time_first_key;
^~~~~~~~~~~~~~
```
The same as #15292.
This PR leverages a workaround and removes the warning.
ACKs for top commit:
laanwj:
ACK e9434ee03e, removes the warning for me (gcc 7.4.0)
kristapsk:
ACK e9434ee03e
Tree-SHA512: 8820a8ba6a75aa6b1ac675a38c883a77f12968b010533b6383180aa66e7e0d570bf6300744903ead91cf9084e5345144959cd6b0cea1b763190b8dd49bacce75
c86bc14408 Make asmap Interpret tolerant of malicious map data (Pieter Wuille)
38c2395d7a Use ASNs for mapped IPv4 addresses correctly (Pieter Wuille)
6f8c937312 Mark asmap const in statistics code (Pieter Wuille)
d58bcdc4b5 Avoid asmap copies in initialization (Pieter Wuille)
Pull request description:
Here are a few things to improve in the asmap implementation. The first two commits are just code improvements. The last one is a bugfix (the exsting code wouldn't correctly apply ASN lookups to mapped/embedded IPv4 addresses).
ACKs for top commit:
practicalswift:
ACK c86bc14408 -- patch looks correct
naumenkogs:
utACK c86bc14
laanwj:
ACK c86bc14408
jonatack:
ACK c86bc14408 code looks correct, built/ran tests, bitcoind with -asmap pointed to asmap/demo.map
Tree-SHA512: 1036f43152754d621bfbecfd3b7c7276e4670598fcaed42a3d275e51fa2cf3653e2c9e9cfa714f6c7719362541510e92171e076ac4169b55a0cc8908b2d514c0
0a50019fde Walk pindexBestHeader back to ChainActive().Tip() if it is invalid (Matt Corallo)
Pull request description:
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.
Places pindexBestHeader is used:
* Various GUI displays of the best header and getblockchaininfo["headers"],
I don't think changing this is bad, and if anything this is less confusing
in the presence of an invalid block.
* IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
isn't some crazy invalid chain is better than the alternative, even in the
case where you are rejecting the current chain due to hardware error (since
hopefully in that case you won't get any new blocks anyway).
* ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
block we're connecting leads to something with nMinimumChainWork (preventing
a user-set assumevalid from having bogus work) and that the block we're
connecting leads to pindexBestHeader (I'm not too worried about this one -
it's nice to "disable" assumevalid if we have a long invalid headers chain,
but I don't see it as a critical protection).
* BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
requested block is within a month of the "current chain". I don't think this
is a meaningful difference, if we're rejecting the current tip we're
trivially fingerprintable anyway, and if the chain really does have a bunch
of invalid crap near the tip, using the best not-invalid header is likely a
better criteria.
* ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
of whether a block request is "historical" for the purpose of bandwidth
limiting. Similarly, I don't see why this is a meaningful change.
* We use pindexBestHeader for requesting missing headers on receipt of a
headers/compact block message or block inv as well as for initial getheaders.
I think this is definitely wrong, using the best not-invalid header for such
requests is much better.
* We use pindexBestHeader to define the "current chain" for deciding when
we're close to done with initial headers sync. I don't think this is a
meaningful change.
* We use pindexBestHeader to decide if initial headers sync has timed out. If
we're rejecting the chain due to hardware error this may result in
additional cases where we ban a peer, but this is already true, so I think
its fine.
ACKs for top commit:
fjahr:
ACK 0a50019fde
kallewoof:
ACK 0a50019fde
ariard:
utACK 0a50019
Tree-SHA512: 2ecfa973a9878a00313ae7ede94a9bd7710e0caf55b544b10bbc46dc463a0478cbaf477e6cdd072356d5a0c5fb3848e9339284af785a2995c20bae8bd23f23e5
bd5a026928 Make UpdateTransactionsFromBlock use Epochs (Jeremy Rubin)
2ccb7cca4a Add Epoch Guards to CTXMemPoolEntry and CTxMemPool (Jeremy Rubin)
Pull request description:
UpdateTransactionsFromBlock is called during a re-org. When a re-org occurs, all of the transactions in the mempool may be descendants from a transaction which is in the pre-reorg block. This can cause us to propagate updates, worst case, to every transaction in the mempool.
Because we construct a `setEntries setChildren`, which is backed by a `std::set`, it is possible that this algorithm is `O(N log N)`.
By using an Epoch visitor pattern, we can limit this to `O(N)` worst case behavior.
Epochs are also less resource intensive than almost any set option (e.g., hash set) because they are allocation free.
This PR is related to https://github.com/bitcoin/bitcoin/pull/17268, it is a small subset of the changes which have been refactored slightly to ease review. If this PR gets review & merge, I will follow up with more PRs (similar to #17268) to improve the mempool
ACKs for top commit:
sdaftuar:
ACK bd5a026928
adamjonas:
Just to summarize for those looking to review - as of bd5a026 there are 3 ACKs (@sdaftuar, @ariard, and @hebasto) and one "looks good" from @ajtowns with no NACKs or any show-stopping concerns raised.
ajtowns:
ACK bd5a026928 (code review)
ariard:
Code review ACK bd5a026
hebasto:
ACK bd5a026928, modulo some nits and a typo.
Tree-SHA512: f0d2291085019ffb4e1119edeb9f4a89c1a572d1cb5b4bdf5743dd0152e721e1935f5155dcae84e1e5bda5ffdf6224c488c1e200bd33bedca9f5ca22d5f5139f
9a299a59cc net: reference instead of copy in BlockConnected range loop (Jon Atack)
Pull request description:
Reference elements in range for loop instead of copying them and
fix Clang `-Wrange-loop-analysis` warning introduced in a029e18
```
net_processing.cpp:1185:25: warning: loop variable 'ptx' of
type 'const std::shared_ptr<const CTransaction>' creates a copy from
type 'const std::shared_ptr<const CTransaction>' [-Wrange-loop-analysis]
for (const auto ptx : pblock->vtx) {
^
net_processing.cpp:1185:14: note: use reference type
'const std::shared_ptr<const CTransaction> &' to prevent copying
for (const auto ptx : pblock->vtx) {
^~~~~~~~~~~~~~~~
1 warning generated.
```
ACKs for top commit:
Empact:
ACK 9a299a59cc
MarcoFalke:
ACK 9a299a59cc
promag:
ACK 9a299a59cc.
elichai:
ACK 9a299a59cc
emilengler:
ACK 9a299a5.
Tree-SHA512: 9284d1b00684877505454a05071212758c8cea083534e2eec09bfc8a9c3059eea811d2008f6a5a678539444f0d5b3134db1bd23da6514b3d3a1440634c8b53be
d3bc184081 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f364 test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20 rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda4 test: remove getaddressinfo label tests (Jon Atack)
c7654af6f8 doc: address pr17578 review feedback (Jon Atack)
Pull request description:
This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.
See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.
Next step: add support for multiple labels.
ACKs for top commit:
jnewbery:
ACK d3bc184081
laanwj:
ACK d3bc184081
meshcollider:
utACK d3bc184081
Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
cb8a86d9f9 gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d10777d gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)
Pull request description:
Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.
ACKs for top commit:
hebasto:
ACK cb8a86d9f9, tested on Linux Mint 19.3.
jonasschnelli:
utACK cb8a86d9f9
Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
3aee10b80b gui: Drop ShutdownWindow dependency to BitcoinGUI (João Barbosa)
61eb058cc1 gui: Drop BanTableModel dependency to ClientModel (João Barbosa)
Pull request description:
`ShutdownWindow::showShutdownWindow` just needs a widget to center the shutdown window and to borrow its title.
ACKs for top commit:
hebasto:
ACK 3aee10b80b, since previous review only suggested change `QWidget` --> `QMainWindow`
jonasschnelli:
utACK 3aee10b80b
Tree-SHA512: e15cb6ee274730bd071d3d97b540c5059e5c655248d69a37c3fd00f2aacc6cfcb36b9a65755718027e15482ec8e5e85534c1dc13d0ddb4e0680df03fbf6571f2
a029e18c2b Use rolling bloom filter of recent block tx's for AlreadyHave() check (Suhas Daftuar)
Pull request description:
In order to determine whether to download or process a relayed transaction, we first try to check whether we already have the transaction -- either in the mempool, in our filter of recently rejected transactions, in our orphan pool, or already confirmed in a block.
Prior to this commit, the heuristic for checking whether a transaction was confirmed in a block is based on whether there's a coin cache entry corresponding to the 0- or 1-index vout of the tx. While that is a quick check, it is very imprecise (eg if those outputs were already spent in another block, we wouldn't detect that the transaction has already been confirmed) -- we can do better by just keeping a rolling bloom filter of the transactions in recent blocks, which will better capture the case of a transaction which has been confirmed and then fully spent.
This should reduce the bandwidth that we waste by requesting transactions which will not be accepted to the mempool.
To avoid relay problems for transactions which have been included in a recent block but then reorged out of the chain, we clear the bloom filter whenever a block is disconnected.
ACKs for top commit:
MarcoFalke:
re-ACK a029e18c2b only stylistic and comment fixups 🍴
sipa:
utACK a029e18c2b
jonatack:
Code review ACK a029e18c2b also built/ran tests and am running bitcoind with mempool debug logging and custom logging. Looked a bit into CRollingBloomFilter and also the mempool median time past checks mentioned above; I don't have a deep understanding of those areas yet but the concept here and changes LGTM. Tests and other optimisations could be added as a follow-up. In favor of seeing this move forward if no major immediate concerns.
Tree-SHA512: 784c9a35bcd3af5db469063ac7d26b4bac430e451e5637a34d8a538c3ffd1433abdd3f06e5584e7a84bfa9e791449e61819397b5a6c7890fa59d78ec3ba507b2
b951b0973c on startup, write config options to debug.log (Larry Ruane)
Pull request description:
When a developer is examining `debug.log` after something goes wrong, it's often useful to know the exact options the failing instance of `bitcoind` was started with. Sometimes the `debug.log` file is all that's available for the analysis. This PR logs the `bitcoin.conf` entries and command-line arguments to `debug.log` on startup.
ACKs for top commit:
MarcoFalke:
ACK b951b0973c🐪
jonatack:
ACK b951b0973c reviewed diff, re-code review, built, ran tests, launched bitcoind and reviewed debug log output, verified value of `str` debug log in the added unit test.
Tree-SHA512: bbca4fb3d49f99261758302bde0b8b67300ccc72e7380b01f1f66a146ae8a008a045df0ca5ca9664caff034d0ee38ea7ef38a50f38374525608c07ba52790358
cc668d06fb tests: Add fuzzing harness for strprintf(...) (practicalswift)
ccc3c76e2b tests: Add fuzzer strprintf to FUZZERS_MISSING_CORPORA (temporarily) (practicalswift)
6ef04912af tests: Update FuzzedDataProvider.h from upstream (LLVM) (practicalswift)
Pull request description:
Add fuzzing harness for `strprintf(…)`.
Update `FuzzedDataProvider.h`.
Avoid hitting some issues in tinyformat (reported upstreams in https://github.com/c42f/tinyformat/issues/70).
---
Found issues in tinyformat:
**Issue 1.** The following causes a signed integer overflow followed by an allocation of 9 GB of RAM (or an OOM in memory constrained environments):
```
strprintf("%.777777700000000$", 1.0);
```
**Issue 2.** The following causes a stack overflow:
```
strprintf("%987654321000000:", 1);
```
**Issue 3.** The following causes a stack overflow:
```
strprintf("%1$*1$*", -11111111);
```
**Issue 4.** The following causes a `NULL` pointer dereference:
```
strprintf("%.1s", (char *)nullptr);
```
**Issue 5.** The following causes a float cast overflow:
```
strprintf("%c", -1000.0);
```
**Issue 6.** The following causes a float cast overflow followed by an invalid integer negation:
```
strprintf("%*", std::numeric_limits<double>::lowest());
```
Top commit has no ACKs.
Tree-SHA512: 9b765559281470f4983eb5aeca94bab1b15ec9837c0ee01a20f4348e9335e4ee4e4fecbd7a1a5a8ac96aabe0f9eeb597b8fc9a2c8faf1bab386e8225d5cdbc18
1b96a3cd1e tests: reset fIsBareMultisigStd after bare-multisig tests (fanquake)
Pull request description:
Fixes: #18015
The bug this fixes is two-part.
1. The `fIsBareMultisigStd` global is being reused by other tests,
such as [script_p2sh_tests(set)](https://github.com/bitcoin/bitcoin/blob/master/src/test/script_p2sh_tests.cpp#L150), after being set to false.
2. The order our tests run in doesn't always? seem to be random,
which meant that the `script_p2sh` tests would only fail if they
were run in an order where the `transaction_tests` ran first,
mutating the `fIsBareMultisigStd` global.
This doesn't seem to happen when running make check, but if you
run `src/test/test_bitcoin and pass --random=99999`, the failure
in `script_p2sh` will occur (on most, but maybe not all systems):
```bash
src/test/test_bitcoin --random=99999
Running 389 test cases...
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[1].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[2].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[3].IsStandard
*** 3 failures are detected in the test module "Bitcoin Core Test Suite"
```
The new test for bare multisig was introduced in #17502.
ACKs for top commit:
Empact:
Code Review ACK 1b96a3cd1e
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/18018/commits/1b96a3cd1ebe725896f59614903184289fe62c
Tree-SHA512: fd7578f9f3faa44d236cd007fc25e31f061acabdb8458559fde0e67d11ab5cafed15305993270c9943a50326574bc5f5301b09494a5b0d2de69e64978093ed45
