In order to override these later, the specific details of how the Read,
Write, Erase, and Exists functions interact with the actual database
file need to go into functions that are not templated.
fa02b47313 refactor: Use AbortError in FatalError (MarcoFalke)
Pull request description:
`FatalError` has been copied from `AbortNode`, so the two should use the same style to avoid confusion.
Follow-up to #18927
ACKs for top commit:
hebasto:
ACK fa02b47313, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2cf6d18a6ffb5c2e5cf54f0a072a7cef6dc7e924152b2fee44e6ff2c6c53bad962afd364eda30d8a73883d656429ea68391090e6a27057e69eaefd7c4dad0a33
f8213c05f0 Add means to handle negative capabilities in thread safety annotations (Hennadii Stepanov)
Pull request description:
This commit is separated from #19238, and it adds support of [Negative Capabilities](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative) in the Clang Thread Safety Analysis attributes.
> Negative requirements are an alternative `EXCLUDES` [`LOCKS_EXCLUDED`] that provide a stronger safety guarantee. A negative requirement uses the `REQUIRES` [`EXCLUSIVE_LOCKS_REQUIRED`] attribute, in conjunction with the ! operator, to indicate that a capability should not be held.
Examples of usage:
- #19238 (for a class)
- https://github.com/hebasto/bitcoin/tree/200610-addrman-tsn (for the whole code base)
ACKs for top commit:
MarcoFalke:
Approach ACK f8213c05f0
vasild:
ACK f8213c05
Tree-SHA512: 86d992826b87579661bd228712ae5ee6acca6f70b885ef7e96458974eac184e4874a525c669607ba6b6c861aa4806409a8792d100e6914c858bcab43d31cfb1b
61c16339da walletdb: Move BDB specific things into bdb.{cpp/h} (Andrew Chow)
8f033642a8 walletdb: moveonly: Move BerkeleyBatch Cursor and Txn funcs to cpp (Andrew Chow)
25a655794a walletdb: move IsWalletLoaded to walletdb.cpp (Andrew Chow)
f6fc5f3849 walletdb: Add IsBDBWalletLoaded to look for BDB wallets specifically (Andrew Chow)
c3538f435a walletdb: Make SpliWalletFilePath non-static (Andrew Chow)
Pull request description:
Moves the BDB specific classes from db.{cpp/h} to bdb.{cpp/h}.
To do this, `SplitWalletFilePath` is first made non-static. Then `IsWalletLoaded` functionality is moved to `IsBDBWalletLoaded` which is called by `IsWalletLoaded`. Then the bulk of db.{cpp/h} is moved to a new file bdb.{cpp/h}.
While doing some moveonly stuff, an additional commit moves the `*Cursor` and `Txn*` implementations out of the header file and into the cpp file.
Part of #18971
ACKs for top commit:
laanwj:
Code review ACK 61c16339da
promag:
Code review ACK 61c16339da.
meshcollider:
utACK 61c16339da
Tree-SHA512: cb676cd34c9cd3c838a4fef230d84711efe4cf0d2eefa64ebfd7f787ddc6f7379db0b29454874ddc46ca7ffee0f18f6f3fb96a85513cd10164048948fd03a80c
fs.cpp:35:17: error: no member named 'strerror' in namespace 'std'
return std::strerror(errno);
~~~~~^
fs.cpp:49:9: error: use of undeclared identifier 'close'
close(fd);
^
2 errors generated.
./interfaces/chain.h:265:55: error: ‘std::function’ has not been declared
virtual void rpcRunLater(const std::string& name, std::function<void()> fn, int64_t seconds) = 0;
^~~
44cc75f80e wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm)
Pull request description:
This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.
ACKs for top commit:
Sjors:
utACK 44cc75f80e (rebased)
fjahr:
re-ACK 44cc75f80e
Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483
313a081b90 [net] Add seed.bitcoin.wiz.biz to DNS seeds (wiz)
Pull request description:
I've created the `seed.bitcoin.wiz.biz` DNS seed for the benefit of the Bitcoin community, and will operate it in accordance with the [Bitcoin DNS seed operator policy](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md). Since this is my first PR to the Bitcoin Core project, I also ACK the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md).
The data for this DNS seed is generated using redundant instances of TheBlueMatt's [dnsseed-rust implementation](https://github.com/TheBlueMatt/dnsseed-rust), which connects to all discoverable Bitcoin nodes to verify their capabilities and speed, and utilizes the full AS-MAP data from my network's BGP tables to select Bitcoin nodes which are fairly distributed across different networks.
As for my qualifications, I currently operate Bitcoin nodes for the [mempool.space](https://mempool.space/) open-source block explorer project (mempool) and the [Bisq Network](https://bisq.network/) open-source P2P trading community (bisq-network). I have 20 years experience as a network engineer, and all of [my Bitcoin nodes](https://bitnodes.io/nodes/?q=AS54415) are hosted on [my own network](https://ipinfo.io/AS54415) across multiple datacenters. For personal references, the current Bitcoin DNS seed operators Emzy and TheBlueMatt can probably vouch for me.
The DNS responses served from this instance are currently served with a TTL of 60 seconds, and the DNS resolvers do not log queries from users. Any inquiries related to the operation of this DNS seed can be sent to <noc@wiz.biz>.
Here is a rough diagram of the `seed.bitcoin.wiz.biz` DNS seed architecture:
ACKs for top commit:
jonasschnelli:
Tested ACK 313a081b90.
laanwj:
ACK 313a081b90
Tree-SHA512: 9e4ea7a929b7888eba748933c1581328aefcba4de503af96f99630d797d794859b22c99999c25c3fc90f6efaed2598f32784d3acea3e428d84bae3aa37f92a25
5527be0627 refactor: Add AbortError alias (Hennadii Stepanov)
d924f2a596 Drop MSG_NOPREFIX flag (Hennadii Stepanov)
083daf7fba Pass bilingual_str argument to AbortNode() (Hennadii Stepanov)
d1cca129b4 refactor: Use bilingual_str::empty() (Hennadii Stepanov)
Pull request description:
This PR is a [followup](https://github.com/bitcoin/bitcoin/issues/16218#issuecomment-625919724) of #16224, and it adds `bilingual_str` type argument support to the `AbortNode()` functions.
ACKs for top commit:
MarcoFalke:
ACK 5527be0627👟
Tree-SHA512: bf8b15b14912b1f672e6e588fffa1e6eb6f00b4b23d15d0ced7f18fbdf76919244427feb7217007fe29617049308e13def893a03a87358db819cca9692f59905
Every `return false` is preceeded by a detailed debug log message to
explain that a disconnect or misbehavior happened. Logging another
generic "FAILED" message seems redundant.
Also, the size of the message and the message type has already been
logged and is thus redundant as well.
Finally, claiming that message processing FAILED seems odd, because the
message was fully processed to the point where it was concluded that the
peer should be either disconnected or marked as misbehaving.
3a10d935ac [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408)
ff8c430c65 [test] test disconnect for filterclear (gzhao408)
1c6b787e03 [netprocessing] disconnect node that sends filterclear (gzhao408)
Pull request description:
Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages.
Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](19e919217e/src/net_processing.cpp (L2218)), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204).
Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them.
ACKs for top commit:
jnewbery:
Code review ACK 3a10d935ac
naumenkogs:
utACK 3a10d93
gillichu:
tested ACK: quick test_runner on macOS [`3a10d93`](3a10d935ac)
MarcoFalke:
re-ACK 3a10d935ac only change is replacing false with true 🚝
Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
The utility is primarily useful to dereference pointer types, which are
known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are
started. Instead of silently relying on that assumption, Assert can be
used to abort the program and avoid UB should the assumption ever be
violated.
-Increasing the banscore and/or banning is too harsh,
just disconnecting is enough.
-Return true from ProcessMessage because we already log
receipt of filterclear and disconnect.
-nodes not serving bloomfilters should disconnect peers
that send filterclear, just like filteradd and filterload
-nodes that want to enable/disable txrelay should use
feefilter
ccf1f6ea24 refactor: Drop ::HasWallets() (João Barbosa)
Pull request description:
Minor follow-up of #19250. The global `HasWallets()` is used only once and at the call site there's already a way to know if any wallet is loaded.
ACKs for top commit:
MarcoFalke:
ACK ccf1f6ea24
hebasto:
ACK ccf1f6ea24, I have reviewed the changes and they look OK, I agree they can be merged.
Tree-SHA512: fb902c045cbd331eaf71716c04734520f2ce7f2b317db510c4ce140162bbc683327b5a40ac860f6cde5add37e069065274d39dfa147fac2091eedec505f2f7eb
fadf6bd04f refactor: Remove unused request.fHelp (MarcoFalke)
fad889cbf0 wallet: Make RPC help compile-time static (MarcoFalke)
Pull request description:
Currently calling `help` on a wallet RPC method will either return `help: unknown command: getnewaddress` or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing.
The fix is split into two commits:
* First, a commit that can be reviewed with the `--color-moved=dimmed-zebra` option and tested with the included test.
* Second, a commit that removes the complicated and confusing code.
ACKs for top commit:
achow101:
re-ACK fadf6bd04f
promag:
Tested ACK fadf6bd04f.
Tree-SHA512: 65d4ff400467f57cb8415c30ce30f814dc76c5c157308b7a7409c59ac9db629e65dfba31cd9c389cfe60a008d3d87787ea0a0e0f2671fd65fd190543c915493d
cf5b8f64b3 tests: Add fuzzing harness for {Read,Write}{LE,BE}{16,32,64} (crypto/common.h) (practicalswift)
4a8181b303 tests: Add std::vector<uint8_t> ConsumeFixedLengthByteVector(FuzzedDataProvider& fuzzed_data_provider, const size_t length) (practicalswift)
Pull request description:
Add fuzzing harness for `{Read,Write}{LE,BE}{16,32,64}` (`crypto/common.h`).
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK cf5b8f64b3
Tree-SHA512: 26412daa6987add1c721ad0348a5a894d68a646e724f328f2db6d9c9358a533481d8888b89d4b0743e9d1c11aa4e0e5341eb4c0d05a4da77b15ab75489327749
f42f5e58f5 refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions (Russell Yanofsky)
Pull request description:
This simplifies control flow and also helps get rid of the ::vpwallets variable in #19101 since EnsureWalletIsAvailable doesn't have access to the request context.
ACKs for top commit:
MarcoFalke:
ACK f42f5e58f5 (reviewed code to check that this is a refactor) 💢
promag:
Tested ACK f42f5e58f5.
Tree-SHA512: eb10685de3db3c1d10c3a797d8da5c8c731e4a8c9024bbb7245929ba767a77a52783a739b8cb1fa7af6fcd233dcf9c8ebbe414eb8b902e2542601aac18625997
After #19176, building the gui on Bionic is failing with:
```bash
CXX qt/qt_libbitcoinqt_a-guiutil.o
qt/bitcoin.cpp: In function 'int GuiMain(int, char**)':
qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
```
The merge commit also failed to compile with the same error:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
6fe989054f refactor: Change Node::initError to take bilingual_str (Wladimir J. van der Laan)
425e7cb8cf refactor: Put`TryParsePermissionFlags` in anonymous namespace (Wladimir J. van der Laan)
77b79fa6ef refactor: Error message bilingual_str consistency (Wladimir J. van der Laan)
Pull request description:
A straightforward and hopefully uncontroversial refactor to improve consistency.
- Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more `InitError(Untranslated(SomeFunction(...)))`.
- Make all functions in `util/error.h` consistently return a `bilingual_str`. We've decided to use this as error message type so let's roll with it.
This has no functional changes: no messages are changed, no new translation messages are defined.
Also make a function static that can be static.
ACKs for top commit:
MarcoFalke:
ACK 6fe989054f🔣
hebasto:
ACK 6fe989054f, tested on Linux Mint 19.3 (x86_64).
Tree-SHA512: 1dd123ef285c4b50bbc429b2f11c9a63aaa669a84955a0a9b8134e9dc141bc38f863f798e8982ac68bbe83170e1067a87d1a87fe7f791928b7914e10bbc2ef8d
2ed54da18a Merge #755: Recovery signing: add to constant time test, and eliminate non ct operators
28609507e7 Add tests for the cmov implementations
73596a85a2 Add ecdsa_sign_recoverable to the ctime tests
2876af4f8d Split ecdsa_sign logic into a new function and use it from ecdsa_sign and recovery
5e1c885efb Merge #754: Fix uninit values passed into cmov
f79a7adcf5 Add valgrind uninit check to cmovs output
05d315affe Merge #752: autoconf: Use ":" instead of "dnl" as a noop
a39c2b09de Fixed UB(arithmetics on uninit values) in cmovs
3a6fd7f636 Merge #750: Add macOS to the CI
5e8747ae2a autoconf: Use ":" instead of "dnl" as a noop
71757da5cc Explictly pass SECP256K1_BENCH_ITERS to the benchmarks in travis.sh
99bd661d71 Replace travis_wait with a loop printing "\a" to stdout every minute
bc818b160c Bump travis Ubuntu from xenial(16.04) to bionic(18.04)
0c5ff9066e Add macOS support to travis
b6807d91d8 Move travis script into a standalone sh file
f39f99be0e Merge #701: Make ec_ arithmetic more consistent and add documentation
39198a03ea Merge #732: Retry if r is zero during signing
59a8de8f64 Merge #742: Fix typo in ecmult_const_impl.h
4e284655d9 Fix typo in ecmult_const_impl.h
f862b4ca13 Merge #740: Make recovery/main_impl.h non-executable
ffef45c98a Make recovery/main_impl.h non-executable
2361b3719a Merge #735: build: fix OpenSSL EC detection on macOS
3b7d26b23c build: add SECP_TEST_INCLUDES to bench_verify CPPFLAGS
84b5fc5bc3 build: fix OpenSSL EC detection on macOS
37ed51a7ea Make ecdsa_sig_sign constant-time again after reverting 25e3cfb
93d343bfc5 Revert "ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign"
7e3952ae82 Clarify documentation of tweak functions.
89853a0f2e Make tweak function documentation more consistent.
41fc785602 Make ec_privkey functions aliases for ec_seckey_negate, ec_seckey_tweak_add and ec_seckey_mul
22911ee6da Rename private key to secret key in public API (with the exception of function names)
5a73f14d6c Mention that value is unspecified for In/Out parameters if the function returns 0
f03df0e6d7 Define valid ECDSA keys in the documentation of seckey_verify
5894e1f1df Return 0 if the given seckey is invalid in privkey_negate, privkey_tweak_add and privkey_tweak_mul
8f814cddb9 Add test for boundary conditions of scalar_set_b32 with respect to overflows
3fec982608 Use scalar_set_b32_seckey in ecdsa_sign, pubkey_create and seckey_verify
9ab2cbe0eb Add scalar_set_b32_seckey which does the same as scalar_set_b32 and also returns whether it's a valid secret key
4f27e344c6 Merge #728: Suppress a harmless variable-time optimization by clang in memczero
01993878bb Add test for memczero()
52a03512c1 Suppress a harmless variable-time optimization by clang in memczero
8f78e208ad Merge #722: Context isn't freed in the ECDH benchmark
ed1b91171a Merge #700: Allow overriding default flags
85b35afa76 Add running benchmarks regularly and under valgrind in travis
ca4906b02e Pass num of iters to benchmarks as variable, and define envvar
02dd5f1bbb free the ctx at the end of bench_ecdh
e9fccd4de1 Merge #708: Constant-time behaviour test using valgrind memtest.
08fb6c4926 Run valgrind_ctime_test in travis
3d2302257f Constant-time behaviour test using valgrind memtest.
96d8ccbd16 Merge #710: Eliminate harmless non-constant time operations on secret data.
0585b8b2ee Merge #718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1
7b50483ad7 Adds a declassify operation to aid constant-time analysis.
34a67c773b Eliminate harmless non-constant time operations on secret data.
ca739cba23 Compile with optimization flag -O2 by default instead of -O3
eb45ef3384 Clarify that a secp256k1_ecdh_hash_function must return 0 or 1
856a01d6ad Merge #714: doc: document the length requirements of output parameter.
d72b9e2483 Merge #682: Remove Java Native Interface
4b48a43106 doc: document the length requirements of output parameter.
1b4d256e2e Merge #713: Docstrings
dabfea7e21 field: extend docstring of secp256k1_fe_normalize
dc7d8fd9e2 scalar: extend docstring of secp256k1_scalar_set_b32
074ab582dd Merge #704: README: add a section for test coverage
acb7f97eb8 README: add a section for test coverage
227a4f2d07 Merge #709: Remove secret-dependant non-constant time operation in ecmult_const.
d567b779fe Clarify comments about use of rzr on ge functions and abs function.
2241ae6d14 Remove secret-dependant non-constant time operation in ecmult_const.
642cd062bd Remove Java Native Interface
83fb1bcef4 Remove -O2 from default CFLAGS because this would override the -O3 flag (see AC_PROG_CC in the Autoconf manual)
ecba8138ec Append instead of Prepend user-CFLAGS to default CFLAGS allowing the user to override default variables
613c34cd86 Remove test in configure.ac because it doesn't have an effect
f45d897101 Merge #703: Overhaul README.md
2e759ec753 Overhaul README.md
d644dda5c9 Merge #689: Remove "except in benchmarks" exception for fp math
bde2a32286 Convert bench.h to fixed-point math
387d723c3f Merge #679: Add SECURITY.md
0db61d25c9 Merge #685: Fix issue where travis does not show the ./tests seed…
a0771d15e6 Explicitly disable buffering for stderr in tests
fb424fbba2 Make travis show the ./tests seed by removing stdout buffering and always cat tests.log after a travis run.
22a6031184 Merge #690: Add valgrind check to travis
544002c008 Merge #678: Preventing compiler optimizations in benchmarks without a memory fence
dd98cc988f travis: Added a valgrind test without endro and enabled recovery+ecdh
b4c1382a87 Add valgrind check to travis
0c774d89e6 Merge #688: Fix ASM setting in travis
5c5f71eea5 Fix ASM setting in travis
e2625f8a98 Merge #684: Make no-float policy explicit
bae1bea3c4 Make no-float policy explicit
78c3836341 Add SECURITY.md
362bb25608 Modified bench_scalar_split so it won't get optimized out
73a30c6b58 Added accumulators and checks on benchmarks so they won't get optimized out
770b3dcd6f Merge #677: Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var
b76142ff25 Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var which was removed in 47045270fa90f81205d989f7107769bce1e71c4d
137d304a6b Merge #647: Increase robustness against UB in secp256k1_scalar_cadd_bit
0d9540b13f Merge #664: Remove mention of ec_privkey_export because it doesn't exist
59782c68b4 Remove mention of ec_privkey_export because it doesn't exist
96cd94e385 Merge #337: variable sized precomputed table for signing
dcb2e3b3ff variable signing precompute table
b4bff99028 Merge #661: Make ./configure string consistent
a467047e11 Make ./configure string consistent
e729cc7f5a Merge #657: Fix a nit in the recovery tests
b64a2e2597 Fix a nit in the recovery tests
e028aa33d3 Merge #650: secp256k1/src/tests.c: Properly handle sscanf return value
f1e11d363d Merge #654: Fix typo (∞)
ef83281c3a Merge pull request #656 from real-or-random/patch-1
556caad2ca Fix typo in docs for _context_set_illegal_callback
0d82732a9a Improve VERIFY_CHECK of overflow in secp256k1_scalar_cadd_bit. This added check ensures that any curve order overflow doesn't go undetected due a uint32_t overflow.
786dfb49f5 Merge #583: JNI: fix use sig array
e95f8ab098 Merge #644: Avoid optimizing out a verify_check
384f55606a Merge #652: README.md: update instruction to run tests
ee56accd47 Merge #651: Fix typo in secp256k1_preallocated.h
7b9b117230 Merge #640: scalar_impl.h: fix includes
d99bec2e21 Merge #655: jni: Use only Guava for hex encoding and decoding
2abcf951af jni: Use only Guava for hex encoding and decoding
271582b3b7 Fix typo
ce6d438266 README.md: update instruction to run tests
b1e68cb8e6 Fix typo in secp256k1_preallocated.h
a11c76c59a secp256k1/src/tests.c: Properly handle sscanf return value
8fe63e5654 Increase robustness against UB. Thanks to elichai2 who noted that the literal '1' is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour. While 'scalar_low_impl''s 'secp256k1_scalar_cadd_bit' is only used for testing purposes and currently the 'bit' parameter is only 0 or 1, it is better to avoid undefined behaviour in case the used domain of 'secp256k1_scalar_cadd_bit' expands.
94ae7cbf83 Moved a dereference so the null check will be before the dereferencing
2cb73b1064 scalar_impl.h: fix includes
fa33017135 Merge #634: Add a descriptive comment for secp256k1_ecmult_const.
ee9e68cd30 Add a descriptive comment for secp256k1_ecmult_const.
d0d738d32d Merge #631: typo in comment for secp256k1_ec_pubkey_tweak_mul ()
6914c25276 typo in comment for secp256k1_ec_pubkey_tweak_mul ()
e541a90ef6 Merge #629: Avoid calling _is_zero when _set_b32 fails.
f34b0c3f35 Merge #630: Note intention of timing sidechannel freeness.
8d1563b0ff Note intention of timing sidechannel freeness.
1669bb2865 Merge #628: Fix ability to compile tests without -DVERIFY.
ecc94abcc8 Merge #627: Guard memcmp in tests against mixed size inputs.
544435fc90 Merge #578: Avoid implementation-defined and undefined behavior when dealing with sizes
143dc6e9ee Merge #595: Allow to use external default callbacks
e49f7991c2 Add missing #(un)defines to base-config.h
77defd2c3b Add secp256k1_ prefix to default callback functions
908bdce64e Include stdio.h and stdlib.h explicitly in secp256k1.c
5db782e655 Allow usage of external default callbacks
6095a863fa Replace CHECKs for no_precomp ctx by ARG_CHECKs without a return
cd473e02c3 Avoid calling secp256k1_*_is_zero when secp256k1_*_set_b32 fails.
6c36de7a33 Merge #600: scratch space: use single allocation
98836b11f0 scratch: replace frames with "checkpoint" system
7623cf2b97 scratch: save a couple bytes of unnecessarily-allocated memory
a7a164f2c6 scratch: rename `max_size` to `size`, document that extra will actually be allocated
5a4bc0bb95 scratch: unify allocations
c2b028a281 scratch space: thread `error_callback` into all scratch space functions
0be1a4ae62 scratch: add magic bytes to beginning of structure
92a48a764d scratch space: use single allocation
40839e21b9 Merge #592: Use trivial algorithm in ecmult_multi if scratch space is small
dcf392027b Fix ability to compile tests without -DVERIFY.
a484e0008b Merge #566: Enable context creation in preallocated memory
0522caac8f Explain caller's obligations for preallocated memory
238305fdbb Move _preallocated functions to separate header
695feb6fbd Export _preallocated functions
814cc78d71 Add tests for contexts in preallocated memory
ba12dd08da Check arguments of _preallocated functions
5feadde462 Support cloning a context into preallocated memory
c4fd5dab45 Switch to a single malloc call
ef020de16f Add size constants for preallocated memory
1bf7c056ba Prepare for manual memory management in preallocated memory
248bffb052 Guard memcmp in tests against mixed size inputs.
36698dcfee Merge #596: Make WINDOW_G configurable
a61a93ff50 Clean up ./configure help strings
2842dc523e Make WINDOW_G configurable
1a02d6ce51 Merge #626: Revert "Merge #620: Install headers automatically"
662918cb29 Revert "Merge #620: Install headers automatically"
14c7dbd444 Simplify control flow in DER parsing
ec8f20babd Avoid out-of-bound pointers and integer overflows in size comparisons
01ee1b3b3c Parse DER-enconded length into a size_t instead of an int
912680ed86 Merge #561: Respect LDFLAGS and #undef STATIC_PRECOMPUTATION if using basic config
91fae3ace0 Merge #620: Install headers automatically
5df77a0eda Merge #533: Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...)
975e51e0d9 Merge #617: Pass scalar by reference in secp256k1_wnaf_const()
735fbde04e Merge #619: Clear a copied secret key after negation
16e86150d0 Install headers automatically
069870d92a Clear a copied secret key after negation
8979ec0d9a Pass scalar by reference in secp256k1_wnaf_const()
84a808598b Merge #612: Allow field_10x26_arm.s to compile for ARMv7 architecture
d4d270a59c Allow field_10x26_arm.s to compile for ARMv7 architecture
248f046611 Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...)
9ab96f7b12 Use trivial algorithm in ecmult_multi if scratch space is small
dbed75d969 Undefine `STATIC_PRECOMPUTATION` if using the basic config
310111e093 Keep LDFLAGS if `--coverage`
74e2dbd68e JNI: fix use sig array
3cb057f842 Fix possible integer overflow in DER parsing
git-subtree-dir: src/secp256k1
git-subtree-split: 2ed54da18add295668ec71c91534b640d2cc029b
- Move the decision whether to translate an error message to where it is
defined. This simplifies call sites: no more `InitError(Untranslated(...))`.
- Make all functions in `util/error.h` consistently return a
`bilingual_str`. We've decided to use this as error message type so
let's roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
b00266fe0c refactor: replace pointers by references within tx_verify.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR gets rid of another unnecessary use of raw pointers, similar to PR #19053 (see also issue #19062 where useful commands for finding potential candidates are listed) but in the tx verification module.
For the functions `CalculateSequenceLocks()` and `SequenceLocks()`, the `prevHeights` vector parameter type is changed to be passed as a reference. Note that there were no checks for null pointers -- if one would pass `nullptr` to one of the functions, the following line would immediately lead to a crash:
dcacea096e/src/consensus/tx_verify.cpp (L32)
ACKs for top commit:
Empact:
Code Review ACK b00266fe0c
Tree-SHA512: 0eb71591467905434082029128bdca4df94988c372af40dca325654f6c002c72a00c73776cb5e72d6de2b2f218649211a5dbf19300a2e01f1841d6034e0f01e0
34645c4dd0 Test txinwitness is accessible on coinbase vin (Rod Vagg)
3e4421070a Expose txinwitness for coinbase in JSON form (Rod Vagg)
Pull request description:
## Rationale
The CLI can provide you with everything about transactions and blocks that you need to reconstruct the block structure and raw block itself **except** for the witness commitment nonce which is stored in the `scriptWitness` of the coinbase and is not printed. You could manually parse the raw `"hex"` fields for transactions if you really wanted to, but this seems to defeat the point of having a JSONification of the raw block/transaction data.
Without the nonce you can't:
1. calculate and validate the witness commitment yourself, you can generate the witness tx merkle root but you don't have the nonce to combine it with
2. reconstruct the raw block form because you don't have `scriptWitness` stack associated with the coinbase (although you know how big it will be and can guess the common case of `[0x000...000]`)
I'm building some archiving tooling for block data and being able to do a validated two-way conversion is very helpful.
## What
This PR simply makes the `txinwitness` field not dependent on whether we are working with the coinbase or not. So you get it for the coinbase as well as the rest.
## Examples
Common case of a `[0x000...000]` nonce: 00000000000000000000140a7289f3aada855dfd23b0bb13bb5502b0ca60cdd7
```json
"vin": [
{
"coinbase": "0368890904c1fe8d5e2f706f6f6c696e2e636f6d2ffabe6d6d5565843a681160cf7b08b1b74ac90a719e6d6ab28c16d336b924f0dc2fcabdc6010000000000000051bf2ad74af345dbe642154b2658931612a70d195e007add0100ffffffff",
"txinwitness": [
"0000000000000000000000000000000000000000000000000000000000000000"
],
"sequence": 4294967295
}
],
...
```
Novel nonce value: 000000000000000000008c31945b2012258366cc600a3e9a3ee0598e8f797731
```json
"vin": [
{
"coinbase": "031862082cfabe6d6d80c099b5e21f4c186d54eb292e17026932e52b1b807fa1380574c5adc1c843450200000000000000",
"txinwitness": [
"5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d"
],
"sequence": 4294967295
}
],
...
```
## Alternatives
This field could be renamed for the coinbase, `"witnessnonce"` perhaps. It could also be omitted when null/zero (`0x000...000`).
## Tests
This didn't break any tests and I couldn't find an obvious way to include a test for this. If this is desired I'd apreicate some pointers.
ACKs for top commit:
MarcoFalke:
ACK 34645c4dd0
Tree-SHA512: b192facc1dfd210a5ec3f0d5d1ac6d0cae81eb35be15eaa71f60009a538dd6a79ab396f218434e7e998563f7f0df2c396cc925cb91619f6841c5a67806148c85
cb38b069b0 util: Don't reference errno when pthread fails. (MIZUTA Takeshi)
Pull request description:
Pthread library does not set errno.
Pthread library's errno is returned by return value.
ACKs for top commit:
practicalswift:
ACK cb38b069b0 -- patch looks correct
MarcoFalke:
review ACK cb38b069b0
hebasto:
ACK cb38b069b0, only squashed commits since the [previous](https://github.com/bitcoin/bitcoin/pull/19194#pullrequestreview-425831739) review.
Tree-SHA512: e6c950e30726e5031db97a7b84c8a9215da5ad3e5d233bcc349f812ad15957ddfe378e26d18339b9e0a5dcac2f50b47a687b87a6a6beaf6139df84f31531321e
fa2c2b50d8 doc: Extract net permissions doc (MarcoFalke)
Pull request description:
Moving the documentation of each flag form the already over-large init.cpp into the net permissions module should clean up the code a bit. Moreover, making the documentation available is also required for an (currently imaginary) `setnetpermissions` RPC.
ACKs for top commit:
Sjors:
re-utACK fa2c2b50d8
Tree-SHA512: c0a75facc9768913c28d2ffcdfaad8d60f7604d5584ee546adaf77d270563558d361aeaf354e49e349aca7e2e80814b27ffc24247e7b4f045c63cbdc079b449f
cc5c0d2299 refactor: Fix formatting of timedata.cpp (Hennadii Stepanov)
c2410ceb84 refactor: Replace RecursiveMutex with Mutex in timedata.cpp (Hennadii Stepanov)
Pull request description:
Only `GetTimeOffset()` and `AddTimeData()` functions lock this mutex. They do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_timeoffset_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK cc5c0d2299 , checked the second commit with --word-diff-regex=. --ignore-all-space -U0 🦉
vasild:
ACK cc5c0d22 verified that recursion is not happening
Tree-SHA512: 38f6df689374d4a1a0e9aedb3ed5e885d8285c4da6b75f9bc84ae036936a159ef8276462db33b4f4dd5c71c6312fa9b45380f7a5726959665bc71dc39031be88
78c8f4fe11 refactor: Replace RecursiveMutex with Mutex in netbase.cpp (Hennadii Stepanov)
Pull request description:
The functions that could lock this mutex, i.e., `{S,G}etProxy()`, `{S,G}etNameProxy()`, `HaveNameProxy()`, `IsProxy()`, do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_proxyinfo_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK 78c8f4fe11 , reviewed with the -W git option 👮
vasild:
ACK 78c8f4fe verified that recursion does not happen
Tree-SHA512: fc077fb371f38af5d05f1383c6bebf9926167c257892936fefd2d4fe6f679ca40124d25099e09f645d8ec266df222f96c5d0f9fd39eddcad15cbde0b427bc205
1a9ef1d398 refactor: Replace RecursiveMutex with Mutex in Shutdown() (Hennadii Stepanov)
Pull request description:
Step by step, going to replace all of the `RecursiveMutex` instances with the `Mutex` ones throughout the code base :)
Not sure if it is possible in all cases though...
This one is a low-hanging fruit.
ACKs for top commit:
MarcoFalke:
ACK 1a9ef1d398 Shutdown is not recursive, so the same thread can never lock twice (UB)
vasild:
ACK 1a9ef1d3 verified manually that `Shutdown()` is not called from places that could be called from inside `Shutdown()`.
Tree-SHA512: 362a507b1a6f97dc351f708224aedbfe4bee03c4398f394d78ee31c24d76a7012ffff0e6766866cd5fd9a8e0d8840f05a2741111fe583aa20d45f0af3df0dcfa
Pthread library does not set errno.
Pthread library's errno is returned by return value.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fac6b9b938 test: Avoid overwriting the NodeContext member of the testing setup (MarcoFalke)
fa16e7816b build: Add -Wshadow-field (MarcoFalke)
Pull request description:
Adding this warning will eliminate unexpected test failures and hard to review code. Moreover, there shouldn't be a use case in Bitcoin Core that relies on fields to be shadowed.
ACKs for top commit:
fanquake:
ACK fac6b9b938 - Warnings compiling fa16e7816b are below. No warnings with fac6b9b938. The `-Wshadow-field` diagnostic has been available in Clang since 5.0.0. It's not available for GCC.
practicalswift:
ACK fac6b9b938 -- patch looks correct
hebasto:
ACK fac6b9b938, tested on Linux Mint 19.3 (x86_64):
Tree-SHA512: 824874ca10877efea7463cf934a2953147f3f99c486f04679426c14ff968975d8652cbba5729bfb7cb2c86c637ade5d1e5b873d611c06bad013a7cad8427e2bf
501e6ab4e7 doc: Add documentation for 'checklevel' argument in 'verifychain' RPC call (Calvin Kim)
Pull request description:
Rationale: When ```bitcoin-cli help verifychain``` is called, the user doesn't get any documentation about the ```checklevel``` argument, leading to issues like #18995.
This PR addresses that issue and adds documentation for what each level does, and that each level includes the checks of the previous levels.
ACKs for top commit:
jonatack:
ACK 501e6ab4e7 `git diff 292ed3c 501e6ab` shows only change since last review is the verifychain RPCHelpMan edit; rebuild and retested manually anyway
MarcoFalke:
ACK 501e6ab4e7🚝
Tree-SHA512: 09239f79c25b5c3022b8eb1f76198ba681305d7e8775038e46becffe5f6a14c572e0c5d06b0723fe9d4a015ec42c9f7ca7b80a2a93df0b1b66f5a84a80eeeeb1
fa9604c46f doc: noban precludes maxuploadtarget disconnects (MarcoFalke)
fa3999fe35 net: Reformat excessively long if condition into multiple lines (MarcoFalke)
Pull request description:
Whitelisting has been replaced by permission flags, so properly document this. See also #10131
ACKs for top commit:
hebasto:
ACK fa9604c46f, I have reviewed the code and it looks OK, I agree it can be merged.
ariard:
ACK fa9604c
Tree-SHA512: 5aee917ab9817719f01ec155487542118e17fa3d145ae7e4bc0e872b2cec39cde9e7fbdee2ae77e9a52700dd8bcc366de4224152e08e709d44d08e0d2f19c613
This simplifies control flow and also helps get rid of the ::vpwallets
variable, because EnsureWalletIsAvailable doesn't have access to the request
context.
4a7253ab6c Remove g_rpc_chain global (Russell Yanofsky)
e783197bf0 refactor: replace RegisterWalletRPCCommands with GetWalletRPCCommands (Russell Yanofsky)
Pull request description:
Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference.
This PR is a followup to #18740 removing the g_rpc_node global.
Some later PRs will follow this up and move more wallet globals to the WalletContext struct.
ACKs for top commit:
MarcoFalke:
ACK 4a7253ab6c🎋
ariard:
Code Review ACK 4a7253a, feel free to ignore comment it's super nit.
Tree-SHA512: 5bb5561c89f81811ca5232a58bf450e230d4218e62471c03227d142395fd36131672e99cb88329b33b9680a235db01e8b9d1c1e2a18288349e57205528deabab
f46b678acf qt: lock cs_main, m_cached_tip_mutex in that order (Vasil Dimov)
Pull request description:
Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in
the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up
in a deadlock.
`ClientModel::m_cached_tip_blocks` is protected by
`ClientModel::m_cached_tip_mutex`. There are two access paths that
lock the two mutexes in opposite order:
```
validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main
validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip()
ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost
...
qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex
```
and
```
qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex
qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash()
interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main
```
From `debug.log`:
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
m_cs_chainstate validation.cpp:2851
(1) cs_main validation.cpp:2868
::mempool.cs validation.cpp:2868
(2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255
Current lock order is:
(2) m_cached_tip_mutex qt/clientmodel.cpp:119
(1) ::cs_main interfaces/node.cpp:200
```
The possible deadlock was introduced in #17993
ACKs for top commit:
jonasschnelli:
Tested ACK f46b678acf
Tree-SHA512: 904f24b39bdc97c4d0ecb897a6980d8d479814535eb167e23105238800ea2f1f85273e3370cf894db58bc597f94c4f2e81fb68d0ff3362d468c16af5ce8f5d78
c4b574899a gui: Add Close All Wallets action (João Barbosa)
f30960adc0 gui: Add closeAllWallets to WalletController (João Barbosa)
Pull request description:
This PR adds the action to close all wallets.
<img width="405" alt="Screenshot 2020-06-01 at 01 06 12" src="https://user-images.githubusercontent.com/3534524/83365986-25a8b980-a3a4-11ea-9613-24dcd8eaa55c.png">
ACKs for top commit:
jonasschnelli:
Tested ACK c4b574899a
Tree-SHA512: 049ad77ac79949fb55f6bde47b583fbf946f4bfaf3d56d768e85f813d814cff0fe326b700f7b5e383cda4af7b5666e13043a6aaeee3798a69fc94385d88ce809
89f9fef1f7 refactor: Specify boost/thread/thread.hpp explicitly (Hennadii Stepanov)
fad8c890f5 txdb: Remove unused boost/thread (MarcoFalke)
faa958bc28 txindex: Remove unused boost/thread (MarcoFalke)
Pull request description:
There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points
However, non-boost threads such as `std::thread` or the `main()` thread can obviously not be interrupted. So remove all unused boost/thread from methods that are never executed in a `boost::thread`.
Most of them were accompanied by a `ShutdownRequested` anyway. So even if the current thread was a `boost::thread`, the interruption point would be redundant. (We only interrupt threads during shutdown)
ACKs for top commit:
fanquake:
ACK 89f9fef1f7
hebasto:
ACK 89f9fef1f7, tested on Linux Mint 19.3 (x86_64), verified shutdown in different scenarios.
Tree-SHA512: 17221dadedf2d107e5bda9e4f371cc4f8ffce6ad27cae41aa2b8f1150d8f1adf23d396585ca4a2dd25b1dc6f0d5c81fecd950d8557966ccb45a6d4a85a331d90
8b3136bd30 refactor: replace CNode pointers by references within net_processing.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR is inspired by a [recent code review comment](https://github.com/bitcoin/bitcoin/pull/19010#discussion_r426954791) on a PR that introduced new functions to the net_processing module. The point of the discussion was basically that whenever we pass something not by value (in the concrete example it was about `CNode*` and `CConnman*`) we should either use
* a pointer (```CType*```) with null pointer check or
* a reference (```CType&```)
To keep things simple, this PR for a first approach
* only tackles `CNode*` pointers
* only within the net_processing module, i.e. no changes that would need adaption in other modules
* keeps the names of the variables as they are
I'm aware that PRs like this are kind of a PITA to review, but I think the code quality would increase if we get rid of pointers without nullptr check -- bloating up the code by adding all the missing checks would be the worse alternative, in my opinion.
Possible follow-up PRs, in case this is received well:
* replace CNode pointers by references for net module
* replace CConnman pointers by references for net_processing module
* ...
ACKs for top commit:
MarcoFalke:
ACK 8b3136bd30🔻
practicalswift:
ACK 8b3136bd30
Tree-SHA512: 15b6a569ecdcb39341002b9f4e09b38ed4df077e3a3a50dfb1b72d98bdc9f9769c7c504f106456aa7748af8591af7bb836b72d46086df715ab116e4ac3224b3b
fa1433ac1b rpc: Remove special case for unknown service flags (MarcoFalke)
Pull request description:
The special case to return a bit as an integer is clumsy and undocumented. Probably also irrelevant because there shouldn't currently be a non-misbehaving client that connects to Bitcoin Core and advertises an unknown service flag.
Thus, simply remove the code.
ACKs for top commit:
laanwj:
ACK fa1433ac1b
Tree-SHA512: 942de6a577a9ee076ce12c92be121617640d53ee8c3424064c45a30a7ff789555d3722a4203670768faf81da2a40adfed3ec5cdeb5da06f04be81ddb53b9db7e
ac94141af0 validation: delay flushing undo files in syncing node case (Karl-Johan Alm)
Pull request description:
Fixes#17890. Replaces #17892.
Data files (`{blk|rev}<number>.dat`) pre-allocate space as they are written, and then trims down to the final size once they move on to the next sequence ("finalized flush"). The code currently assumes (incorrectly) that blk and rev files finish at the same time, but because blk files are written as blocks come in, and rev files are written in block height order, rev files end up being written to for awhile after moving on to the next block file, resulting in pre-allocation and waste of up to 1 MB of space per rev file.
The exact point at which rev file writing finishes is the highest height block found inside the corresponding block file, which is already available in the CBlockFileInfo vector. This PR moves finalized flushing of undo files to to directly after the undo data for the previous block file has been written.
There is a branch with annotation that demonstrates how this is handling flushing here: https://github.com/kallewoof/bitcoin/tree/200124-rev-files-annotated
ACKs for top commit:
vasild:
ACK ac94141af (no changes in the code since ed34e00da).
fjahr:
Code review re-ACK ac94141af0
jonatack:
Code review ACK ac94141af0
Tree-SHA512: 1d4e3b3d1d99bd7ebe7a2f632b1231146dd4f9f993c54db3a4090d9c086d95d2e4c327fd936066392b3afc6277b8f3a908d5c5993d4c8e49f72b92a417716dd2
eea8114657 build: Enable unreachable-code-loop-increment (Jonathan Schoeller)
d15db4b1fc refactor: Fix unreachable code in init arg checks (Jonathan Schoeller)
Pull request description:
Closes: #19017
In #19015 it's been suggested that we add some new compiler warnings to our build. Some of these, such as `-Wunreachable-code-loop-increment`, generate warnings. We'll likely want to fix these up if we're going to turn these warnings on.
```shell
init.cpp:969:5: warning: loop will run at most once (loop increment never executed) [-Wunreachable-code-loop-increment]
for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
^~~
1 warning generated.
```
aa8d76806c/src/init.cpp (L968-L972)
To fix this, collect all errors, and output them in a single error message after the loop completes. This resolves the unreachable code warning, and avoids popup hell that could result from outputting a seperate message for each error or warning one by one.
ACKs for top commit:
laanwj:
Code review ACK eea8114657
hebasto:
re-ACK eea8114657, only suggested changes applied since the [previous](https://github.com/bitcoin/bitcoin/pull/19131#pullrequestreview-421772387) review.
Tree-SHA512: 2aa3ceb7fab581b6ba2580900668388d8eba1c3001c8ff9c11c1f4a9a10fbc37f30e590249862676858446e3f4950140a252953ba1643ba3bfd772f8eae20583
789e9dd3aa validation: use std::chrono in IsCurrentForFeeEstimation() (fanquake)
47be28c8bc validation: use std::chrono in CChainState::FlushStateToDisk() (fanquake)
Pull request description:
Probably up for debate as to which type is used for the constants. Personally, swapping these to hours is more readable.
ACKs for top commit:
MarcoFalke:
ACK 789e9dd3aa
jonatack:
ACK 789e9dd3aa
Tree-SHA512: f4a25cbd00a49a54b7783a1f588be83706dd2a475cecb5c2e8b97b2d4b27c0955a7454d7486f2454e96351c44f233b300c4f4b9ca62fc7336277f10da34dd5c3
fab860aed4 fuzz: Stop nodes in process_message* fuzzers (MarcoFalke)
6666c828e0 fuzz: Give CNode ownership to ConnmanTestMsg in process_message fuzz harness (MarcoFalke)
Pull request description:
Background is that I saw an integer overflow in net_processing
```
#30629113 REDUCE cov: 25793 ft: 142917 corp: 3421/2417Kb lim: 4096 exec/s: 89 rss: 614Mb L: 1719/4096 MS: 1 EraseBytes-
net_processing.cpp:977:25: runtime error: signed integer overflow: 2147483624 + 100 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior net_processing.cpp:977:25 in
net_processing.cpp:985:9: runtime error: signed integer overflow: -2147483572 - 100 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior net_processing.cpp:985:9 in
```
Telling from the line numbers, it looks like `nMisbehavior` wrapped around.
Fix that by calling `StopNodes` after each exec, which should clear the node state and thus `nMisbehavior`.
ACKs for top commit:
practicalswift:
ACK fab860aed4
Tree-SHA512: 891c081d5843565d891aec028b6c27ef3fa39bc40ae78238e81d8f784b4d4b49cb870998574725a5159dd03aeeb2e0b9bc3d3bb51d57d1231ef42e3394b2d639
ActivateBestChain (ABC) is only called in the "msghand" or one of the
RPC threads, neither of which is a boost::thread. However, ABC is also
called in ThreadImport (which currently happens to be a boost::thread).
In all cases, the interruption_point is redundant with the breakpoint in
ABC that triggers when ShutdownRequested()
VerifyDB is only called in the main thread ("init") or one of the RPC
threads, neither of which is a boost::thread.
7eaf86d3bf trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) in https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd and 7e89994133 from https://github.com/bitcoin/bitcoin/pull/16624, causing issue https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard (ariard)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bf
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bf🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
fa1c74fd03 wallet: Remove unused boost::thread_interrupted (MarcoFalke)
fa7b885f51 walletdb: Remove unsed boost/thread (MarcoFalke)
5555d978b0 wallet: Make PeriodicFlush uninterruptible (MarcoFalke)
Pull request description:
The `boost::this_thread::interruption_point()` in the code base currently block the replacement of `boost::thread` with `std::thread`. [1]
Remove them from the wallet because they are either unused or useless.
The feature to interrupt a periodic flush is useless because all wallets have just been flushed 9ccaee1d5e/src/init.cpp (L194) and another flush should be a noop. Also, they will be flushed again shortly after 9ccaee1d5e/src/init.cpp (L285), so even if repeated flushes weren't a noop, doing 3 instead of 2 shouldn't matter too much at this point. Also, the wallet is flushed every two seconds in the worst case, so if this is an expensive operation, that period should be readjusted. (Or bdb should be removed altogether #18916)
[1] Replacement of `boost::thread` with `std::thread` should happen because:
* The boost thread dependency is slow to compile
* Boost thread is less maintained than the standard lib
* Boost thread is mostly redundant to the standard lib
* Global interruption points via exceptions are hard to keep track of during review and easy to get wrong during runtime (e.g. accidental `catch (...)`)
ACKs for top commit:
fanquake:
ACK fa1c74fd03
Tree-SHA512: b166619256de2ef4325480fa1367f68bc9371ad785ec503aed61eab41ba61f1a9807aab25451a24efda3db64855c9ba0025645b98bc58557bc3ec56c5b3297d0
152e8baf08 Use salted hasher instead of nonce in sigcache (Jeremy Rubin)
5495fa5850 Add Hash Padding Microbenchmarks (Jeremy Rubin)
Pull request description:
This PR replaces nonces in two places with pre-salted hashers.
The nonce is chosen to be 64 bytes long so that it forces the SHA256 hasher to process the chunk. This leaves the next 64 (or 56 depending if final chunk) open for data. In the case of the script execution cache, this does not make a big performance improvement because the nonce was already properly padded to fit into one buffer, but does make the code a little simpler. In the case of the sig cache, this should reduce the hashing overhead slightly because we are less likely to need an additional processing step.
I haven't benchmarked this, but back of the envelope it should reduce the hashing by one buffer for all combinations except compressed public keys with compact signatures.
ACKs for top commit:
ryanofsky:
Code review ACK 152e8baf08. No code changes, just rebase since last review and expanded commit message
Tree-SHA512: b133e902fd595cfe3b54ad8814b823f4d132cb2c358c89158842ae27daee56ab5f70cde2585078deb46f77a6e7b35b4cc6bba47b65302b7befc2cff254bad93d
fa9c675591 Limit scope of all global std::once_flag (MarcoFalke)
Pull request description:
`once_flag` is a helper (as the name might suggest) to execute a callable only once. Thus, the scope of the flag does never need to extend beyond where the callable is called. Typically this is function scope.
Move all the flags to function scope to
* simplify code review
* avoid mistakes where similarly named flags are accidentally exchanged
* avoid polluting the global scope
ACKs for top commit:
hebasto:
ACK fa9c675591, tested on Linux Mint 19.3 (x86_64).
promag:
Code review ACK fa9c675591.
Tree-SHA512: 095a0c11d93d0ddcb82b3c71676090ecc7e3de3d5e7a2a63ab2583093be279242acac43523bbae2060b4dcfa8f92b54256a0e91fbbae78fa92d2d49e9db62e57
4f49d5222e gui, refactor: Register Qt meta types in application constructor (João Barbosa)
Pull request description:
Removes a warning when running `QT_QPA_PLATFORM=cocoa src/qt/test/test_bitcoin-qt`.
ACKs for top commit:
jonasschnelli:
Re utACK 4f49d5222e
hebasto:
ACK 4f49d5222e, tested on macOS 10.15.5.
Tree-SHA512: e931a022ba83cb0ef04d82544ebd9b18242f8fc2b41443afce4d5c4222f222e8b3517bdb484a1a4f61377c5dceca067d8ccf250da3a727299448e54bec33ed6e
Building with -Wunreachable-code-loop-increment causes a warning
due to always returning on the first iteration of the loop that
outputs errors on invalid args.
Collect all errors, and output them in a single error message
after the loop completes, resolving the warning and avoiding
popup hell by outputting a seperate message for each error.
Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in
the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up
in a deadlock.
`ClientModel::m_cached_tip_blocks` is protected by
`ClientModel::m_cached_tip_mutex`. There are two access paths that
lock the two mutexes in opposite order:
```
validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main
validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip()
ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost
...
qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex
```
and
```
qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex
qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash()
interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main
```
From `debug.log`:
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
m_cs_chainstate validation.cpp:2851
(1) cs_main validation.cpp:2868
::mempool.cs validation.cpp:2868
(2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255
Current lock order is:
(2) m_cached_tip_mutex qt/clientmodel.cpp:119
(1) ::cs_main interfaces/node.cpp:200
```
The possible deadlock was introduced in
https://github.com/bitcoin/bitcoin/pull/17993
facef3d413 doc: Explain that anyone can work on good first issues, move text to CONTRIBUTING.md (MarcoFalke)
fae2fb2a19 doc: Expand section on Getting Started (MarcoFalke)
100000d1b2 doc: Add headings to CONTRIBUTING.md (MarcoFalke)
fab893e0ca doc: Fix unrelated typos reported by codespell (MarcoFalke)
Pull request description:
Some random doc changes:
* Add sections to docs, so that they can be linked to
* Explain that anyone (even maintainers) are allowed to work on good first issues
* Expand section on Getting Started slightly
ACKs for top commit:
hebasto:
ACK facef3d413
fanquake:
ACK facef3d413
Tree-SHA512: 8998e273a76dbf4ca77e79374c14efe4dfcc5c6df6b7d801e1e1e436711dbe6f76b436f9cbc6cacb45a56827babdd6396f3bd376a9426ee7be3bb9b8a3b8e383
f898ef65c9 tests: Add fuzzing harness for functions in script/sign.h (practicalswift)
c91d2f0615 tests: Add fuzzing harness for functions in script/sigcache.h (practicalswift)
d3d8adb79f tests: Add fuzzing harness for functions in script/interpreter.h (practicalswift)
fa80117cfd tests: Add fuzzing harness for functions in script/descriptor.h (practicalswift)
43fb8f0ca3 tests: Add fuzzing harness for functions in script/bitcoinconsensus.h (practicalswift)
8de72711c6 tests: Fill fuzzing coverage gaps for functions in script/script.h, script/script_error.h and script/standard.h (practicalswift)
c571ecb071 tests: Add fuzzing helper functions ConsumeDataStream, ConsumeTxDestination and ConsumeUInt160 (practicalswift)
Pull request description:
Add fuzzing harnesses for functions in `script/`:
* Add fuzzing helper functions `ConsumeDataStream` and `ConsumeUInt160`
* Fill fuzzing coverage gaps for functions in `script/script.h`, `script/script_error.h` and `script/standard.h`
* Add fuzzing harness for functions in `script/bitcoinconsensus.h`
* Add fuzzing harness for functions in `script/descriptor.h`
* Add fuzzing harness for functions in `script/interpreter.h`
* Add fuzzing harness for functions in `script/sigcache.h`
* Add fuzzing harness for functions in `script/sign.h`
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK f898ef65c9🔉
Tree-SHA512: f6e77b34dc79f23de5fa9e38ac06e6554b5b946ec3e9a67e2bd982e60aca37ce844f785457ef427a5e3b45e31c305456bca8587cc9f4a0b50b3852e39726eb04
9e36067d8c [test] Add test for cfilters. (Jim Posen)
11106a4722 [net processing] Message handling for getcfilters. (Jim Posen)
e535670726 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5 [refactor] Pass CNode and CConnman by reference (John Newbery)
Pull request description:
Support `getcfilters` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
Empact:
re-Code Review ACK 9e36067d8c
MarcoFalke:
re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
jkczyz:
ACK 9e36067d8c
fjahr:
Code review ACK 9e36067d8c
Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
9a19c9ada5 Always define the raii_event_tests test suite (Craig Andrews)
Pull request description:
The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test.
This improves upon 95f97f4 actually fixing https://github.com/bitcoin/bitcoin/issues/9493
ACKs for top commit:
MarcoFalke:
ACK 9a19c9ada5🎹
Tree-SHA512: 3c42f17a9b5d56c8841f3aa9ac19da91c10aff210026266f31f7eb98a62528740d7c518c121452b68e8f801d6c80ecfb627d137ec6ed533289fa3beb08b4f176
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
f871f15c9d scripted-diff: replace gArgs with argsman (glowang)
357f02bf29 Create a local class inherited from BasicTestingSetup with a localized args manager and put it into the getarg_tests namespace (glowang)
Pull request description:
Replaced the global argsManager gArgs with a locally defined one in getarg_tests. This is to avoid confusion in arg settings between the test's ArgsManager and the #18804
ACKs for top commit:
MarcoFalke:
ACK f871f15c9d
ryanofsky:
Code review ACK f871f15c9d. Changes look good and thanks for updating. In future would recommend using clang-format-diff and following [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) notes, because it's atypical to indent namespace content, or indent protected keywords or put spaces around ::. Also it's fragile to define test setup class in a namespace, but test setup methods outside of the namespace and inside the test fixture instead. Would be simpler to just define the testing setup completely before using it without a namespace like: 8ad5f1c376/src/test/rpc_tests.cpp (L23) and it would have been a slightly smaller change too.
Tree-SHA512: 016594639396d60667fadec8ea80ef7af634fbb2014c704f02406fe3251c5362757c21f1763d8bdb94ca4a3026ab9dc786a92a9a934efc8cd807655d9deee779
189ae0c38b util: dedup code in callers of serviceFlagToStr() (Vasil Dimov)
fbacad1880 util: simplify the interface of serviceFlagToStr() (Vasil Dimov)
Pull request description:
Don't take two redundant arguments in `serviceFlagToStr()`.
Introduce `serviceFlagsToStr()` which takes a mask (with more than one
bit set) and returns a vector of strings.
As a side effect this fixes an issue introduced in
https://github.com/bitcoin/bitcoin/pull/18165 due to which the GUI could
print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]`
instead of `NETWORK & WITNESS`.
ACKs for top commit:
MarcoFalke:
ACK 189ae0c38b
jonasschnelli:
Tested ACK 189ae0c38b
Tree-SHA512: 000c490f16ebbba04458c62ca4ce743abffd344d375d95f5bbd5008742012032787655db2874b168df0270743266261dccf1693761906567502dcbac902bda50
Don't take two redundant arguments in `serviceFlagToStr()`.
As a side effect this fixes an issue introduced in
https://github.com/bitcoin/bitcoin/pull/18165 due to which the GUI could
print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]`
instead of `NETWORK & WITNESS`.
da73f1513a qt: Fix shutdown when waitfor* cmds are called from RPC console (Hennadii Stepanov)
Pull request description:
On master (7eed413e72), if the GUI has been started with`-server=1`, `bitcoin-qt` hangs on shutdown during calling any of the `waitfor*` commands in the GUI RPC console.
This PR suggests minimal changes to fix this bug.
Fix#17495
ACKs for top commit:
jonasschnelli:
utACK da73f1513a
Tree-SHA512: 469f5332945a5f2c57d19336cda5df79b123ccc494aea6d58a85eb1293be52708b2b9c5bb6bc2c402a90b7b4e9e8d7ab8fe84cf201cf7ce612c9290c57e43681
9760293ce6 wallet: Fix for exported confirmation field in payment to self transactions (Ben Carman)
Pull request description:
Closes#3455
ACKs for top commit:
jonasschnelli:
Tested ACK 9760293ce6
Tree-SHA512: 8207768771ad787f716b966c4aa7aeef2da8a602e32e3510e41c7b49ec5ec679a3835d248be5016d4b37764f9914846f7c41c11cf48cddb617cb7ef831318fd7
96954d1794 DNS seeds: don't query DNS while network is inactive (Anthony Towns)
fa5894f7f5 DNS seeds: wait for 5m instead of 11s if 1000+ peers are known (Anthony Towns)
Pull request description:
Changes the logic for querying DNS seeds: after this PR, if there's less than 1000 entries in addrman, it will still usually query DNS seeds after 11s (unless the first few peers tried mostly succeed), but if there's more than 1000 entries it won't try DNS seeds until 5 minutes have passed without getting multiple outbound peers. (If there's 0 entries in addrman, it will still immediately query the DNS seeds). Additionally, delays querying DNS seeds while the p2p network is not active.
Fixes#15434
ACKs for top commit:
fanquake:
ACK 96954d1794 - Ran some tests of different scenarios. More documentation is being added in #19084.
ariard:
Tested ACK 96954d1, on Debian 9.1. Both MANY_PEERS/FEW_PEERS cases work.
Sjors:
tACK 96954d1 (rebased on master) on macOS 10.15.4. It found it useful to run with `-debug=addrman` and change `DNSSEEDS_DELAY_MANY_PEERS` to something lower to test the behaviour, as well as renaming `peers.dat` to test the peer threshold.
naumenkogs:
utACK 96954d1794
Tree-SHA512: 73693db3da73bf8e76c3df9e9c82f0a7fb08049187356eac2575c4ffa455f76548dd1c86a11fc6beea8a3baf0ba020e047bebe927883c731383ec72442356005
8e08d00598 qt: Use parent-child relation to manage lifetime of OptionsModel object (Hennadii Stepanov)
Pull request description:
Both `BitcoinApplication` and `OptionsModel` classes are derived from the `QObject` class, therefore a parent-child relation could be established to manage the lifetime of an `OptionsModel` object:
5236b2e267/src/qt/optionsmodel.cpp (L29-L30)
This PR does not change behavior.
ACKs for top commit:
jonasschnelli:
utACK 8e08d00598
promag:
ACK 8e08d00598.
Tree-SHA512: 0223dddf5ba28b0bfaefeda1b03b4ff95bf7e7d0c1e7b32368171e561813e22129f2a664f09279fa3b4fa63259b7680d55aa3fe66db9c7ae0039b7f529777ec3
c31bc5bcfd Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function (Luke Dashjr)
cea91a1e40 Bugfix: GUI: Use unsigned long long type to avoid implicit conversion of MSB check (Luke Dashjr)
Pull request description:
Side effect: this results in the RPC showing unknown service bits as "UNKNOWN[n]" like the GUI.
Note that there is no common mask-to-`vector<string>` function because both GUI and RPC would need to iterate through it to convert to their desired target formats.
ACKs for top commit:
jonasschnelli:
utACK ~~cea91a1e40e12029140ebfba969ce3ef2965029c~~ c31bc5bcfd
Tree-SHA512: 32c7ba8ac7ef2d4087f4f317447ae93a328ec9fb9ad81301df2fbaeeb21a3db7a503187a369552b05a9414251b7cf8e15bcde74c1ea2ef36591ea7ffb6721f60
a06e845e82 BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals. (furszy)
2f867203b0 Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals. (furszy)
Pull request description:
Rationale:
The height based polling in the GUI is an issue on chain reorgs. Any new tip signal with the same height as the one that it's cached in the model was not triggering the GUI update (interpreting it as the same same block when it could receive a different one). Ending up with bad information presented in the GUI.
This PR essentially changes the last cached height to be a last cached block hash.
---
Old historical information of this PR.
As the tip height is cached and updated via signaling in clientModel, there is no need to continue locking `cs_main` on every balance poll (`m_node.getNumBlocks()` method call).
Extra topic:
Would suggest to change the `cachedNumBlocks` field inside `walletModel` to a more understandable name, maybe `nLastBalanceUpdateHeight`.
And finally, this will have the equal height reorg issue mentioned [here](https://github.com/bitcoin/bitcoin/pull/17905#issuecomment-577324304), whatever is presented to fix it, this should use the same flow too.
**[Edit - 24/01/2020]**
Have added #[17905](https://github.com/bitcoin/bitcoin/pull/17905#issuecomment-577324304) comment fix here too.
ACKs for top commit:
jonasschnelli:
utACK a06e845e82 - it would be great to have QT unit tests (in this case for a reorg) that either automatically inspect the window content based on accessibility and tests for expected values or at least allow for quick manual re-testing (screenshots, automatically create UI situations).
hebasto:
re-ACK a06e845e82, suggested style changes implemented since the [previous](https://github.com/bitcoin/bitcoin/pull/17993#pullrequestreview-417249705) review.
ryanofsky:
Code review ACK a06e845e82. A lot of changes since the last review: rebase after sync_state introduction #18152 and tryGetBalances revert #18587, reverting getLastBlockTime change, fixing spacing and initializations and renaming some variables
Tree-SHA512: 835e587a8296df9899cccd7b3e598a5970942b640e432e6a32de0b4eaea5b40f9271258f089ec033595311707b74a0f7187ecf8ed397c713e1153e2714072975
c4ea501e96 qt: Hide non PKHash-Addresses in signing address book (Emil Engler)
Pull request description:
[Video Demo](https://www.youtube.com/watch?v=T-Rp2pFRmzY)
This PR hides all non PKHash addresses in the signing GUI in the Address Book when it is opened through the signing dialog, as non PKHash addresses are useless there.
ACKs for top commit:
jonasschnelli:
Code Review ACK c4ea501e96
Tree-SHA512: e321d45e15534b2d68da5a1297b1c7551cdd784f03203f54c9385c2ce0bb2b7316c09f9e8c3eb41bfa1e7207ecc94c8ed08f012e2d6c117b803996ade26feb2f
e8fa0a3d20 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson)
Pull request description:
Fixes#18622
A bug in WSL means that fcntl does not exclusively lock files, allowing multiple instances of bitcoin to use the same datadir. If we instead use flock, it works correctly. Passes Travis, but testing on some OS variety would be sensible.
From what I can tell, flock and fcntl don't work with each other on linux, so it would still be possible to run a node with this code change and a node before it with the same datadir (this isn't true for Mac/FreeBSD). flock also doesn't support NFS on MacOS and linux<2.6.12 while fcntl did. See here for example: https://gavv.github.io/articles/file-locks/
If changing to flock for all systems is inadvisable, it would also be possible to just detect WSL and use flock when on that platform to avoid the bug.
ACKs for top commit:
laanwj:
Code review ACK e8fa0a3d20
Tree-SHA512: ca1009e171970101f1dc2332c5e998717aee00eebc80bb586b826927a74bd0d4c94712e46d1396821bc30533d76deac391b6e1c406c406865661f57fa062c702
After commit d0ebd93 the scheduler itself no longer cares if the
serviceQueue is run in a std::thread or boost::thread. Change the
documentation to std::thread because we switched to C++11.
Replace with RPC request reference to new WalletContext struct similar to the
existing NodeContext struct and reference.
This PR is a followup to 25ad2c623ahttps://github.com/bitcoin/bitcoin/pull/18740 removing the g_rpc_node global.
Some later PRs will follow this up and move more wallet globals to the
WalletContext struct.
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
5478d6c099 logging: thread safety annotations (Anthony Towns)
e685ca1992 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns)
a788789948 test/checkqueue_tests: thread safety annotations (Anthony Towns)
479c5846f7 rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns)
8b5af3d4c1 net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns)
de7c5f41ab wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns)
c3cf2f5501 rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns)
Pull request description:
In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes.
This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations.
It's based on top of #16112, and turns the thread safety comments included there into annotations.
It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well.
ACKs for top commit:
MarcoFalke:
ACK 5478d6c099🗾
hebasto:
re-ACK 5478d6c099, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review.
ryanofsky:
Code review ACK 5478d6c099. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard
Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
c57f03ce17 refactor: Replace const char* to std::string (Calvin Kim)
Pull request description:
Rationale: Addresses #19000
Some functions should be returning std::string instead of const char*.
This commit changes that.
Main benefits/reasoning:
1. The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)
2. All call sites convert to string anyway
ACKs for top commit:
MarcoFalke:
re-ACK c57f03ce17 (no changes since previous review) 🚃
Empact:
Fair enough, Code Review ACK c57f03ce17
practicalswift:
ACK c57f03ce17 -- patch looks correct
hebasto:
re-ACK c57f03ce17
Tree-SHA512: 9ce99bb38fe399b54844315048204cafce0f27fd8f24cae357fa7ac6f5d8094d57bbf5f5c1f5878a65f2d35e4a3f95d527eb17f49250b690c591c0df86ca84fd
84ae0578b6 Add release notes about salvage changes (Andrew Chow)
ea337f2d03 Move RecoverKeysOnlyFilter into RecoverDataBaseFile (Andrew Chow)
9ea2d258b4 Move RecoverDatabaseFile and RecoverKeysOnlyFilter into salvage.{cpp/h} (Andrew Chow)
b426c7764d Make BerkeleyBatch::Recover and WalletBatch::RecoverKeysOnlyFilter standalone (Andrew Chow)
2741774214 Expose a version of ReadKeyValue and use it in RecoverKeysOnlyFilter (Andrew Chow)
ced95d0e43 Move BerkeleyEnvironment::Salvage into BerkeleyBatch::Recover (Andrew Chow)
07250b8dce walletdb: remove fAggressive from Salvage (Andrew Chow)
8ebcbc85c6 walletdb: don't automatically salvage when corruption is detected (Andrew Chow)
d321046f4b wallet: remove -salvagewallet (Andrew Chow)
cdd955e580 Add basic test for bitcoin-wallet salvage (Andrew Chow)
c87770915b wallettool: Add a salvage command (Andrew Chow)
Pull request description:
Removes the `-salvagewallet` startup option and adds a `salvage` command to the `bitcoin-wallet` tool. As such, `-salvagewallet` is removed. Additionally, the automatic salvage that is done if the wallet file fails to load is removed.
Lastly the salvage code entirely is moved out entirely into `bitcoin-wallet` from `walletdb.{cpp/h}` and `db.{cpp/h}`.
ACKs for top commit:
jonatack:
ACK 84ae0578b6 feedback taken, and compared to my previous review, the bitcoin-wallet salvage command now seems to run and it exits without raising. The new test passes at both 9454105 and 84ae057 so as a sanity check I'd agree there is room for improvement, if possible.
MarcoFalke:
re-ACK 84ae0578b6🏉
Empact:
Code Review ACK 84ae0578b6
ryanofsky:
Code review ACK 84ae0578b6. Lot of small changes since previous review: added verify step before salvage, added basic test in new commit, removed unused scanstate variable and warnings parameter, tweaked various comments and strings, moved fsuccess variable declaration
meshcollider:
Concept / light code review ACK 84ae0578b6
Tree-SHA512: 05be116b56ecade1c58faca1728c8fe4b78f0a082dbc2544a3f7507dd155f1f4f39070bd1fe90053444384337bc48b97149df5c1010230d78f8ecc08e69d93af
This only changes network serialization. Disk serialization does not
include the filter_type and is defined in
ReadFilterFromDisk()/WriteFilterToDisk().
71f016c6eb Remove old serialization primitives (Pieter Wuille)
92beff15d3 Convert LimitedString to formatter (Pieter Wuille)
ef17c03e07 Convert wallet to new serialization (Pieter Wuille)
65c589e45e Convert Qt to new serialization (Pieter Wuille)
Pull request description:
This is the final step 🥳 of the serialization improvements extracted from #10785.
It converts the LimitedString wrapper to a new-style formatter, and updates the wallet and Qt code to use the new serialization framework. Finally all remaining old primitives are removed.
ACKs for top commit:
jonatack:
ACK 71f016c6eb reviewed diff, builds/tests/re-fuzzed.
laanwj:
Code review ACK 71f016c6eb
Tree-SHA512: d952194bc73259f6510bd4ab1348a1febbbf9862af30f905991812fb0e1f23f15948cdb3fc662be54d648e8f6d95b11060055d2e7a8c2cb5bf008224870b1ea1
90eb027204 doc: Add and fix comments about never destroyed objects (Hennadii Stepanov)
26c093a995 Replace thread_local g_lockstack with a mutex-protected map (Hennadii Stepanov)
58e6881bc5 refactor: Refactor duplicated code into LockHeld() (Hennadii Stepanov)
f511f61dda refactor: Add LockPair type alias (Hennadii Stepanov)
8d8921abd3 refactor: Add LockStackItem type alias (Hennadii Stepanov)
458992b06d Prevent UB in DeleteLock() function (Hennadii Stepanov)
Pull request description:
Tracking our instrumented mutexes (`Mutex` and `RecursiveMutex` types) requires that all involved objects should not be destroyed until after their last use. On master (ec79b5f86b) we have two problems related to the object destroying order:
- the function-local `static` `lockdata` object that is destroyed at [program exit](https://en.cppreference.com/w/cpp/utility/program/exit)
- the `thread_local` `g_lockstack` that is destroyed at [thread exit](https://en.cppreference.com/w/cpp/language/destructor)
Both cases could cause UB at program exit in so far as mutexes are used in other static object destructors.
Fix#18824
ACKs for top commit:
MarcoFalke:
re-ACK 90eb027204, only change is new doc commit 👠
ryanofsky:
Code review ACK 90eb027204 because all the changes look correct and safe. But I don't know the purpose of commit 26c093a995 "Replace thread_local g_lockstack with a mutex-protected map (5/6)." It seems like it could have a bad impact on debug performance, and the commit message and PR description don't give a reason for the change.
Tree-SHA512: 99f29157fd1278994e3f6eebccedfd9dae540450f5f8b980518345a89d56b635f943a85b20864cef087027fd0fcdb4880b659ef59bfe5626d110452ae22031c6
fa756928c3 rpc: Make gettxoutsetinfo/GetUTXOStats interruptible (MarcoFalke)
fa7fc5a8e0 rpc: factor out RpcInterruptionPoint from dumptxoutset (MarcoFalke)
Pull request description:
Make it interruptible, so that shutdown doesn't block for up to one hour.
Fixes (partially) #13217
ACKs for top commit:
Empact:
Code Review ACK fa756928c3
laanwj:
Code review ACK fa756928c3
Tree-SHA512: 298261e0ff7d79fab542b8f6828cc0ac451cbafe396d5f0816c9d36437faba1330f5c4cb2a25c5540e202bfb9783da6ec858bd453056ce488d21e36335d3d42c
f9b22e3bdb tests: Add fuzzing harness for CCoinsViewCache (practicalswift)
Pull request description:
Add fuzzing harness for `CCoinsViewCache`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK f9b22e3bdb📫
Tree-SHA512: 4fa79aab683875eef128b672cf199909c86e4d2ed7c406f006fa27a546dafc9cb0061c4de5e660e622458072f1dab69dbf6b6b03d5b863f81c5710bf4cee6c0c
5308c97cca [test] Add test for cfheaders (Jim Posen)
f6b58c1506 [net processing] Message handling for getcfheaders. (Jim Posen)
3bdc7c2d39 [doc] Add comment for m_headers_cache (John Newbery)
Pull request description:
Support `getcfheaders` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
jkczyz:
ACK 5308c97cca
MarcoFalke:
re-ACK 5308c97cca , only change is doc related 🗂
theStack:
ACK 5308c97cca🚀
Tree-SHA512: 240fc654f6f634c191d9f7628b6c4801f87ed514a1dd55c7de5d454d4012d1c09509a2d5a246bc7da445cd920252b4cd56a493c060cdb207b04af4ffe53b95f7
Returning by const value is only meaningful in a specific circumstance around
user defined types. In this case, the const is not enforcing any restrictions
on the call site, so is misleading.
Instead of having these be class static functions, just make them be
standalone. Also removes WalletBatch::Recover which just passed through
to BerkeleyBatch::Recover.
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
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
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
- 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
412d5fe879 QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr)
2abe8cc3b7 Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr)
Pull request description:
#16060 removed CSV & segwit from versionbits, breaking the "rules" key returned by GBT.
Without this, miners don't know they're mining segwit blocks, and should fall back to pre-segwit block creation.
ACKs for top commit:
sipa:
ACK 412d5fe879
jnewbery:
Tested ACK 412d5fe879.
Tree-SHA512: 825d72e257dc0dd4941f2fe498d8d4f4f2a21b9505cd21a8f9eb7fb5d6d7dd9219347928cf90bb57a777920ce24295859763e64fa8a22ebb58fc2380f80f5615
a0d0f1c6c3 refactor: Remove Node:: queries from GUI (Hennadii Stepanov)
06d519f0b4 qt: Add SynchronizationState enum to signal parameter (Hennadii Stepanov)
3c709aa69d refactor: Remove Node::getReindex() call from GUI (Hennadii Stepanov)
1dab574edf refactor: Pass SynchronizationState enum to GUI (Hennadii Stepanov)
2bec309ad6 refactor: Remove unused bool parameter in RPCNotifyBlockChange() (Hennadii Stepanov)
1df77014d8 refactor: Remove unused bool parameter in BlockNotifyGenesisWait() (Hennadii Stepanov)
Pull request description:
This PR is a followup of #18121 and:
- addresses confusion about GUI notification throttling conditions (**luke-jr**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378552386), **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378975960))
- removes `isInitialBlockDownload()` call from the GUI back to the node (on macOS). See: **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#pullrequestreview-357730284)
ACKs for top commit:
jonasschnelli:
Core Review ACK a0d0f1c6c3 (modulo [question](https://github.com/bitcoin/bitcoin/pull/18152#pullrequestreview-414140601)).
ryanofsky:
Code review ACK a0d0f1c6c3. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)
Tree-SHA512: b6a712a710666e763aeee0d5440de1391a4c6c8f7fa661888773e1ba59e9e0f83654ee384d4edc704031be7eb25616e5eca2a6e26058d3efb7f64c47f9ed7316
fa243be1dc log: Remove "No rpcpassword set" from logs (MarcoFalke)
Pull request description:
rpcpassword is deprecated and not recommended anymore. So remove it from the logs, which indicate that an rpcpassword should be set and cause confusion. See #18998.
ACKs for top commit:
ryanofsky:
Code review ACK fa243be1dc. New log message makes more sense
elichai:
Re Code Review ACK (Checked the diff) fa243be1dc
Tree-SHA512: de3e0800a204b15a59a59a7e6f345013ee9d38e8c5d0c9a94d6142780faa9cce672ed358c7571f53c1eb843bf5afb0b7bcbfd289d3b9e2e0bf8ff2fd361e98a9
2896c412fa Do not answer GETDATA for to-be-announced tx (Pieter Wuille)
f2f32a3dee Push down use of cs_main into FindTxForGetData (Pieter Wuille)
c6131bf407 Abstract logic to determine whether to answer tx GETDATA (Pieter Wuille)
Pull request description:
This PR intends to improve transaction-origin privacy.
In general, we should try to not leak information about what transactions we have (recently) learned about before deciding to announce them to our peers. There is a controlled transaction dissemination process that reveals our transactions to peers that has various safeguards for privacy (it's rate-limited, delayed & batched, deterministically sorted, ...), and ideally there is no way to test which transactions we have before that controlled process reveals them. The handling of the `mempool` BIP35 message has protections in this regard as well, as it would be an obvious way to bypass these protections (handled asynchronously after a delay, also deterministically sorted).
However, currently, if we receive a GETDATA for a transaction that we have not yet announced to the requester, we will still respond to it if it was announced to *some* other peer already (because it needs to be in `mapRelay`, which only happens on the first announcement). This is a slight privacy leak.
Thankfully, this seems easy to solve: `setInventontoryTxToSend` keeps track of the txids we have yet to announce to a peer - which almost(*) exactly corresponds to the transactions we know of that we haven't revealed to that peer. By checking whether a txid is in that set before responding to a GETDATA, we can filter these out.
(*) Locally resubmitted or rebroadcasted transactions may end up in setInventoryTxToSend while the peer already knows we have them, which could result in us incorrectly claiming we don't have such transactions if coincidentally requested right after we schedule reannouncing them, but before they're actually INVed. This is made even harder by the fact that filterInventoryKnown will generally keep known reannouncements out of setInventoryTxToSend unless it overflows (which needs 50000 INVs in either direction before it happens).
The condition for responding now becomes:
```
(not in setInventoryTxToSend) AND
(
(in relay map) OR
(
(in mempool) AND
(old enough that it could have expired from relay map) AND
(older than our last getmempool response)
)
)
```
ACKs for top commit:
naumenkogs:
utACK 2896c41
ajtowns:
ACK 2896c412fa
amitiuttarwar:
code review ACK 2896c412fa
jonatack:
ACK 2896c412fa per `git diff 2b3f101 2896c41` only change since previous review is moving the recency check up to be verified first in `FindTxForGetData`, as it was originally in 353a391 (good catch), before looking up the transaction in the relay pool.
jnewbery:
code review ACK 2896c412fa
Tree-SHA512: e7d5bc006e626f60a2c108a9334f3bbb67205ace04a7450a1e4d4db1d85922a7589e0524500b7b4953762cf70554c4a08eec62c7b38b486cbca3d86321600868
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.
cd34038cbd Switch from Optional<T> to std::optional<T> (C++17). Run clang-format. (practicalswift)
fb559c1170 tests: Fill fuzzing coverage gaps for functions in util/translation.h (practicalswift)
b74f3d6c45 tests: Fill fuzzing coverage gaps for functions in consensus/validation.h (practicalswift)
c0bbf8193d tests: Fill fuzzing coverage gaps for functions in primitives/block.h (practicalswift)
Pull request description:
* Fill fuzzing coverage gaps for functions in `consensus/validation.h`
* Fill fuzzing coverage gaps for functions in `primitives/block.h`
* Fill fuzzing coverage gaps for functions in `util/translation.h`
* Switch from `Optional<T>` to `std::optional<T>` (C++17). Run `clang-format`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: d6aa4634c3953ade173589a8239bd230eb317ef897835a8557acb73df01b25e5e17bf46f837838e59ec04c1f3d3b7d1309ba68c8a264d17b938215512c9e6085
It is no longer necessary to wait for IBD to be complete before setting
a HD seed. This check was originally to ensure that restoring an old
seed on an out of sync node would scan the entire blockchain and thus
not miss transactions that involved keys that were not in the keypool.
This was necessary as once the seed was changed, no further keys would
be derived from the old seed(s).
As we are now topping up inactive seeds as we find those keys to be
used, this check is no longer necessary. During IBD, each time we
find a used key belonging to an inactive hd seed, we will still generate
more keys from that inactive seed.
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous
wallet notification and status behavior for transactions removed from the
mempool because they conflict with transactions in a block. The behavior was
accidentally changed in two `CWallet::BlockConnected` updates:
a31be09bfd and
7e89994133 from
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive
cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard <ariard@student.42.fr>
0000ea3265 test: Add test for GetRandMillis and GetRandMicros (MarcoFalke)
fa0e5b89cf Add templated GetRandomDuration<> (MarcoFalke)
Pull request description:
A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter:
```cpp
template <typename D>
D GetRandDur(const D& duration_max)
{
return D{GetRand(duration_max.count())};
}
BOOST_AUTO_TEST_CASE(util_time_GetRandTime)
{
std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1});
// Want seconds to be in range [0..1hour), but always get zero :((((
BOOST_CHECK_EQUAL(rand_hour.count(), 0);
}
```
Luckily `std::common_type` is already specialised in the standard lib for `std::chrono::duration` (https://en.cppreference.com/w/cpp/chrono/duration/common_type). And its effect seem to be that the call site must always specify the template argument explicitly.
So instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use.
ACKs for top commit:
laanwj:
Code review ACK 0000ea3265
promag:
Code review ACK 0000ea3265.
jonatack:
ACK 0000ea3 thanks for the improved documentation. Code review, built, ran `src/test/test_bitcoin -t random_tests -l test_suite` for the new unit tests, `git diff fa05a4c 0000ea3` since previous review:
hebasto:
ACK 0000ea3265 with non-blocking [nit](https://github.com/bitcoin/bitcoin/pull/18781#discussion_r424924671).
Tree-SHA512: e89d46e31452be6ea14269ecbbb2cdd9ae83b4412cd14dff7d1084283092722a2f847cb501e8054394e4a3eff852f9c87f6d694fd008b3f7e8458cb5a3068af7
050e2ee6f2 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan)
Pull request description:
Fix the following error in travis:
test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor
const BlockValidationState state_dummy;
ACKs for top commit:
MarcoFalke:
Tested ACK 050e2ee6f2 on xenial with clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
fanquake:
ACK 050e2ee6f2 - I see why we didn't hit this on master. We are installing the `clang-8` packages for the tsan job. However on the 0.20 branch we are still just installing `clang`, which is 3.8.
Tree-SHA512: 8a1d57289dbe9895ab79f81ca87b4fd723426b8d72f3a34bec9553226fba69f6dc19551c1f1d52db6c4b2652164a02ddc60f3187c3e2ad7bcacb0aaca7fa690a
