fac0cf6e55 rpc: Do not advertise dumptxoutset as a way to flush the chainstate (MarcoFalke)
Pull request description:
The help message leaks several implementation details: leveldb and flush.
Neither of them are relevant to the end user and I don't see why we should make them part of the API contract.
ACKs for top commit:
laanwj:
ACK fac0cf6e55
Tree-SHA512: 273fb85dc5be6cdccf17c43f183fa83c57d0a1cbb30555838f32c074218b713a753930009f6c98c85659421f2285f09c0a713b22f7e34d446e56737ac03870f7
06e434d7d9 test: fix message for ECC_InitSanityCheck test (fanquake)
Pull request description:
OpenSSL is long gone.
ACKs for top commit:
laanwj:
Good catch. ACK 06e434d7d9
Tree-SHA512: 1a920fd6493e0374ca00633407e0130f987b136bc68d2062402747bda16a1e588a12bd8b0b8cdef828c9911f210386cfbdb25d478cb9b684d52769d197032064
692f8307fc test: add test for witness commitment index (fanquake)
06442549f8 validation: Add minimum witness commitment size constant (fanquake)
Pull request description:
16101de5f3: Per [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Commitment_structure), the witness commitment structure is at least 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
54f8c48d6a: As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone tries to "optimize" GetWitnessCommitmentIndex() by returning
early.
ACKs for top commit:
MarcoFalke:
ACK 692f8307fc🌵
jonatack:
Code review ACK 692f830
ajtowns:
ACK 692f8307fc
jnewbery:
utACK 692f8307fc
laanwj:
ACK 692f8307fc
Tree-SHA512: 7af3fe4b8a52fea2cdd0aec95f7bb935351a77b73d934bc88d6625a3503311b2a062cba5190b2228f97caa76840db3889032d910fc8e318ca8e7810a8afbafa0
ff6549c3c8 fix: update rest info on block size and json (Chris Abrams)
Pull request description:
Addressing the ambiguous block size text in rest docs: https://github.com/bitcoin/bitcoin/issues/18703
Also makes sure to let developers know there is `.json` option for the rest output format.
ACKs for top commit:
MarcoFalke:
ACK ff6549c3c8
promag:
ACK ff6549c3c8.
Tree-SHA512: 9ef93c1432d650b1f9599778ba092c1ca5b084a537af257078e1c713c76c5d3a4cc4b1ede8a2489964be8ed0303ad8bea58c1cb4759bbb9b24dbdebfec8001d3
c31cbe7cfe Add C++17 test to Travis (Pieter Wuille)
7829685e27 Add configure option for c++17 (Pieter Wuille)
0fbde488b2 Support conversion between Spans of compatible types (Pieter Wuille)
7cbfebbf3d Update ax_cxx_compile_stdcxx.m4 (Pieter Wuille)
Pull request description:
This adds a `--enable-c++17` option to the configure script, fixes the only C++17 incompatibility (with a commit taken from #18468), and adds a Travis test for it.
This is all off by default, and release builds remain C++11.
It implements the first step of the plan in https://github.com/bitcoin/bitcoin/issues/16684.
ACKs for top commit:
elichai:
tACK c31cbe7cfe
practicalswift:
Tested ACK c31cbe7cfe
hebasto:
ACK c31cbe7cfe, tested on Linux Mint 19.3 both C++11 and C++17 modes. Compiled and passed tests locally.
Tree-SHA512: a4b00776dbceef9c12abbb404c6bcd48f7916ce24c8c7a14116355f64e817578b7fcddbedd5ce435322319d1e4de43429b68553f4d96d970c308fe3e3e59b9d1
182dbdf0f4 util: Detect posix_fallocate() instead of assuming (Vasil Dimov)
Pull request description:
Don't assume that `posix_fallocate()` is available on Linux and not
available on other operating systems. At least FreeBSD has it and we
are not using it.
Properly check whether `posix_fallocate()` is present and use it if it
is.
ACKs for top commit:
laanwj:
ACK 182dbdf0f4
Tree-SHA512: f9ed4bd661f33ff6b2b1150591e860b3c1f44e12b87c35e870d06a7013c4e841ed2bf17b41ad6b18fe471b0b23a4b5e42cf1400637180888e0bc56c254fe0766
32b6b386a5 tests: Sort fuzzing harnesses (practicalswift)
e1e181fad1 tests: Add fuzzing coverage for JSONRPCTransactionError(...) and RPCErrorFromTransactionError(...) (practicalswift)
103b6ecce0 tests: Add fuzzing coverage for TransactionErrorString(...) (practicalswift)
dde508b8b0 tests: Add fuzzing coverage for ParseFixedPoint(...) (practicalswift)
1532259fca tests: Add fuzzing coverage for FormatHDKeypath(...) and WriteHDKeypath(...) (practicalswift)
90b635e84e tests: Add fuzzing coverage for CHECK_NONFATAL(...) (practicalswift)
a4e3d13df6 tests: Add fuzzing coverage for StringForFeeReason(...) (practicalswift)
a19598cf98 tests: Add fuzzing harness for functions in system.h (ArgsManager) (practicalswift)
Pull request description:
Add fuzzing harnesses for various classes/functions in `util/`.
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: d27947220850c2a202c7740f44140c17545f45522596912452ccab0c2f5379abeb07cc769982c7855cb465059425206371a2b75ee1c285b03984161c9619d0b0
7918c1b019 test: Add CreateWalletFromFile test (Russell Yanofsky)
Pull request description:
Add unit test calling CreateWalletFromFile, which isn't currently called from other unit tests, with some basic checks to make sure it rescans and registers for notifications correctly.
Motivation for this change was to try to write a test that would fail without the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8 from https://github.com/bitcoin/bitcoin/pull/16426, but succeed with it:
ef8c6ca607/src/wallet/wallet.cpp (L3978-L3986)
However, writing a full test for the race condition that call prevents isn't possible without the locking changes from #16426. So this PR just adds as much test coverage as is possible now.
This new test is also useful for https://github.com/bitcoin/bitcoin/pull/15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates.
ACKs for top commit:
MarcoFalke:
ACK 7918c1b019
jonatack:
ACK 7918c1b019
Tree-SHA512: 44035aee698ecb722c6039d061d8fac2011e9da0b314e4aff19be1d610b53cacff99016b34d6b84669bb3b61041b2318d9d8e3363658f087802ae4aa36ca17b8
fabe44e815 bench: Start nodes with -nodebuglogfile (MarcoFalke)
Pull request description:
For benchmarking we don't want to depend on the speed of the disk or the amount of debug logging
ACKs for top commit:
fanquake:
ACK fabe44e815 - This makes some of these benchmarks significantly faster to run. MempoolEviction total runtime is down from ~46s to 11s on my machine:
Tree-SHA512: d99700901650325896b9115d20b84a27042152f46266f595bf7ea1414528c0b346f4e707a12ee8b8ba99c35cf155e645e67971c1b2a679c4e609c400ff8b08ae
fcb7261625 Prevent valgrind false positive in rest_blockhash_by_height (Russell Yanofsky)
Pull request description:
A bad interaction between valgrind and clang 6.0.0-1ubuntu2 with -O2 optimizations makes valgrind misleadingly imply C++ code is reading an uninitialized blockheight value in `rest_blockhash_by_height` just because that's what clang optimized code is doing. The C++ code looks like:
```c++
int32_t blockheight;
if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
```
while the optimized code looks like:
```
0x00000000000f97ab <+123>: callq 0x4f8860 <ParseInt32(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int*)>
0x00000000000f97b0 <+128>: mov 0xc(%rsp),%ebx
0x00000000000f97b4 <+132>: test %ebx,%ebx
0x00000000000f97b6 <+134>: js 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>
0x00000000000f97bc <+140>: xor $0x1,%al
0x00000000000f97be <+142>: jne 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>
```
During the rest_interface.py test:
eef90c14ed/test/functional/interface_rest.py (L266)
when `height_str` is empty, `ParseInt32` returns false and `blockheight` value is never assigned. The optimized code reads the uninitialized `blockheight` value in `0xc(%rsp)` before the checking the `ParseInt32` return value in `%al`, which is harmless, but triggers the following error from valgrind:
```
==30660== Thread 13 b-httpworker.2:
==30660== Conditional jump or move depends on uninitialised value(s)
==30660== at 0x2017B6: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:614)
==30660== by 0x2041B9: operator() (rest.cpp:670)
==30660== by 0x2041B9: std::_Function_handler<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&), StartREST(util::Ref const&)::$_1>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (std_function.h:301)
==30660== by 0x3EC994: operator() (std_function.h:706)
==30660== by 0x3EC994: HTTPWorkItem::operator()() (httpserver.cpp:55)
==30660== by 0x3ED16D: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:114)
==30660== by 0x3E9168: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:342)
==30660== by 0x3EDAAA: __invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:60)
==30660== by 0x3EDAAA: __invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:95)
==30660== by 0x3EDAAA: _M_invoke<0, 1, 2> (thread:234)
==30660== by 0x3EDAAA: operator() (thread:243)
==30660== by 0x3EDAAA: std:🧵:_State_impl<std:🧵:_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> > >::_M_run() (thread:186)
==30660== by 0x64256DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==30660== by 0x54876DA: start_thread (pthread_create.c:463)
==30660== by 0x6DC888E: clone (clone.S:95)
==30660== Uninitialised value was created by a stack allocation
==30660== at 0x20173A: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:608)
==30660==
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:_ZL24rest_blockhash_by_heightRKN4util3RefEP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
fun:operator()
fun:_ZNSt17_Function_handlerIFbP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEZ9StartRESTRKN4util3RefEE3$_1E9_M_invokeERKSt9_Any_dataOS1_S9_
fun:operator()
fun:_ZN12HTTPWorkItemclEv
fun:_ZN9WorkQueueI11HTTPClosureE3RunEv
fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi
fun:__invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
fun:__invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
fun:_M_invoke<0, 1, 2>
fun:operator()
fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvP9WorkQueueI11HTTPClosureEiES6_iEEEEE6_M_runEv
obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25
fun:start_thread
fun:clone
}
```
This is a known bad interaction between clang and valgrind. The clang optimized code is correct but valgrind has no way of knowing that accessing the uninitialized value isn't a problem. Issue has been reported previously:
- https://bugs.llvm.org/show_bug.cgi?id=32604#c4
- https://github.com/Z3Prover/z3/issues/972
This commit just sets blockheight to -1 as a workaround.
This change was originally made in 41d5d651594c6c939add7a58b7e30c97dccdf24a from #18740 to fix the travis error there (https://travis-ci.org/github/bitcoin/bitcoin/jobs/678453061#L7157) but MarcoFalke suggested https://github.com/bitcoin/bitcoin/pull/18740#discussion_r414772851 moving to a new PR, since apparently the error's been seen on travis previously
ACKs for top commit:
MarcoFalke:
ACK fcb7261625
practicalswift:
ACK fcb7261625
Tree-SHA512: ec8abf45bd3d6c6e0e7e404d0b2a749efd43910619b84b0b5fe7dab22881598d1011a0f3ff2e146bf46320b63eb152bf63c62c06f1ab84c35dd640abc468f18f
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone trys to "optimise" GetWitnessCommitmentIndex() be returning
early.
Per BIP 141, the witness commitment structure is atleast 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
fa501700e9 wallet: Recommned absolute path for dumpwallet (MarcoFalke)
Pull request description:
Avoids misunderstandings such as #9564
ACKs for top commit:
kristapsk:
utACK fa501700e9
Tree-SHA512: f675ef607992857ffeb556a2945b5436a70b39c5d83f05a8be15a6fccc84cbe9d03e52f8239e28d159e41ed7c6f119b7a38e8ab327029f04609f63c559c12c49
Add unit test calling CreateWalletFromFile, which isn't currently called from
other unit tests, with some basic checks to make sure it rescans and registers
for notifications correctly.
Motivation for this change was to try to write a test that would fail without
the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8
from https://github.com/bitcoin/bitcoin/pull/16426, but succeed with it:
ef8c6ca607/src/wallet/wallet.cpp (L3978-L3986)
However, writing a full test for the race condition that call prevents isn't
possible without the locking changes from #16426. So this PR just adds as much
test coverage as is possible now.
This new test is also useful for https://github.com/bitcoin/bitcoin/pull/15719,
since it detects the stale notifications.transactionAddedToMempool notifications
that PR eliminates.
A bad interaction between valgrind and clang 6.0.0-1ubuntu2 with -O2
optimizations makes valgrind misleadingly imply C++ code is reading an
uninitialized blockheight value in rest_blockhash_by_height just because that's
what clang optimized code is doing. The C++ code looks like:
int32_t blockheight;
if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
while the optimized code looks like:
0x00000000000f97ab <+123>: callq 0x4f8860 <ParseInt32(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int*)>
0x00000000000f97b0 <+128>: mov 0xc(%rsp),%ebx
0x00000000000f97b4 <+132>: test %ebx,%ebx
0x00000000000f97b6 <+134>: js 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>
0x00000000000f97bc <+140>: xor $0x1,%al
0x00000000000f97be <+142>: jne 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>
During the rest_interface.py test:
self.test_rest_request("/blockhashbyheight/", ret_type=RetType.OBJ, status=400)
when height_str is empty, ParseInt32 returns false and blockheight value is
never assigned. The optimized code reads the uninitialized blockheight value
in 0xc(%rsp) before the checking the ParseInt32 return value in %al, which is
harmless, but triggers the following error from valgrind:
==30660== Thread 13 b-httpworker.2:
==30660== Conditional jump or move depends on uninitialised value(s)
==30660== at 0x2017B6: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:614)
==30660== by 0x2041B9: operator() (rest.cpp:670)
==30660== by 0x2041B9: std::_Function_handler<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&), StartREST(util::Ref const&)::$_1>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (std_function.h:301)
==30660== by 0x3EC994: operator() (std_function.h:706)
==30660== by 0x3EC994: HTTPWorkItem::operator()() (httpserver.cpp:55)
==30660== by 0x3ED16D: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:114)
==30660== by 0x3E9168: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:342)
==30660== by 0x3EDAAA: __invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:60)
==30660== by 0x3EDAAA: __invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:95)
==30660== by 0x3EDAAA: _M_invoke<0, 1, 2> (thread:234)
==30660== by 0x3EDAAA: operator() (thread:243)
==30660== by 0x3EDAAA: std:🧵:_State_impl<std:🧵:_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> > >::_M_run() (thread:186)
==30660== by 0x64256DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==30660== by 0x54876DA: start_thread (pthread_create.c:463)
==30660== by 0x6DC888E: clone (clone.S:95)
==30660== Uninitialised value was created by a stack allocation
==30660== at 0x20173A: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:608)
==30660==
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:_ZL24rest_blockhash_by_heightRKN4util3RefEP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
fun:operator()
fun:_ZNSt17_Function_handlerIFbP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEZ9StartRESTRKN4util3RefEE3$_1E9_M_invokeERKSt9_Any_dataOS1_S9_
fun:operator()
fun:_ZN12HTTPWorkItemclEv
fun:_ZN9WorkQueueI11HTTPClosureE3RunEv
fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi
fun:__invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
fun:__invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
fun:_M_invoke<0, 1, 2>
fun:operator()
fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvP9WorkQueueI11HTTPClosureEiES6_iEEEEE6_M_runEv
obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25
fun:start_thread
fun:clone
}
This is a known bad interaction between clang and valgrind. The clang optimized
code is correct but valgrind has no way of knowing that accessing the
uninitialized value isn't a problem. Issue has been reported previously:
https://bugs.llvm.org/show_bug.cgi?id=32604#c4https://github.com/Z3Prover/z3/issues/972
This commit just sets blockheight to 0 as a workaround.
223588b1bb Add a --descriptors option to various tests (Andrew Chow)
869f7ab30a tests: Add RPCOverloadWrapper which overloads some disabled RPCs (Andrew Chow)
cf06062859 Correctly check for default wallet (Andrew Chow)
886e0d75f5 Implement CWallet::IsSpentKey for non-LegacySPKMans (Andrew Chow)
3c19fdd2a2 Return error when no ScriptPubKeyMan is available for specified type (Andrew Chow)
388ba94231 Change wallet_encryption.py to use signmessage instead of dumpprivkey (Andrew Chow)
1346e14831 Functional tests for descriptor wallets (Andrew Chow)
f193ea889d add importdescriptors RPC and tests for native descriptor wallets (Hugo Nguyen)
ce24a94494 Add IsLegacy to CWallet so that the GUI knows whether to show watchonly (Andrew Chow)
1cb42b22b1 Generate new descriptors when encrypting (Andrew Chow)
82ae02b165 Be able to create new wallets with DescriptorScriptPubKeyMans as backing (Andrew Chow)
b713baa75a Implement GetMetadata in DescriptorScriptPubKeyMan (Andrew Chow)
8b9603bd0b Change GetMetadata to use unique_ptr<CKeyMetadata> (Andrew Chow)
72a9540df9 Implement FillPSBT in DescriptorScriptPubKeyMan (Andrew Chow)
84b4978c02 Implement SignMessage for descriptor wallets (Andrew Chow)
bde7c9fa38 Implement SignTransaction in DescriptorScriptPubKeyMan (Andrew Chow)
d50c8ddd41 Implement GetSolvingProvider for DescriptorScriptPubKeyMan (Andrew Chow)
f1ca5feb4a Implement GetKeypoolOldestTime and only display it if greater than 0 (Andrew Chow)
586b57a9a6 Implement ReturnDestination in DescriptorScriptPubKeyMan (Andrew Chow)
f866957979 Implement GetReservedDestination in DescriptorScriptPubKeyMan (Andrew Chow)
a775f7c7fd Implement Unlock and Encrypt in DescriptorScriptPubKeyMan (Andrew Chow)
bfdd073486 Implement GetNewDestination for DescriptorScriptPubKeyMan (Andrew Chow)
58c7651821 Implement TopUp in DescriptorScriptPubKeyMan (Andrew Chow)
e014886a34 Implement SetupGeneration for DescriptorScriptPubKeyMan (Andrew Chow)
46dfb99768 Implement writing descriptorkeys, descriptorckeys, and descriptors to wallet file (Andrew Chow)
4cb9b69be0 Implement several simple functions in DescriptorScriptPubKeyMan (Andrew Chow)
d1ec3e4f19 Add IsSingleType to Descriptors (Andrew Chow)
953feb3d27 Implement loading of keys for DescriptorScriptPubKeyMan (Andrew Chow)
2363e9fcaa Load the descriptor cache from the wallet file (Andrew Chow)
46c46aebb7 Implement GetID for DescriptorScriptPubKeyMan (Andrew Chow)
ec2f9e1178 Implement IsHDEnabled in DescriptorScriptPubKeyMan (Andrew Chow)
741122d4c1 Implement MarkUnusedAddresses in DescriptorScriptPubKeyMan (Andrew Chow)
2db7ca765c Implement IsMine for DescriptorScriptPubKeyMan (Andrew Chow)
db7177af8c Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWallet (Andrew Chow)
78f8a92910 Implement SetType in DescriptorScriptPubKeyMan (Andrew Chow)
834de0300c Store WalletDescriptor in DescriptorScriptPubKeyMan (Andrew Chow)
d8132669e1 Add a lock cs_desc_man for DescriptorScriptPubKeyMan (Andrew Chow)
3194a7f88a Introduce WalletDescriptor class (Andrew Chow)
6b13cd3fa8 Create LegacyScriptPubKeyMan when not a descriptor wallet (Andrew Chow)
aeac157c9d Return nullptr from GetLegacyScriptPubKeyMan if descriptor wallet (Andrew Chow)
96accc73f0 Add WALLET_FLAG_DESCRIPTORS (Andrew Chow)
6b8119af53 Introduce DescriptorScriptPubKeyMan as a dummy class (Andrew Chow)
06620302c7 Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it (Andrew Chow)
Pull request description:
Introducing the wallet of the glorious future (again): native descriptor wallets. With native descriptor wallets, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor. Native descriptor wallets will be optional for now and can only be created by using `createwallet`.
Descriptor wallets will store descriptors, master keys from the descriptor, and descriptor cache entries. Keys are derived from descriptors on the fly. In order to allow choosing different address types, 6 descriptors are needed for normal use. There is a pair of primary and change descriptors for each of the 3 address types. With the default keypool size of 1000, each descriptor has 1000 scriptPubKeys and descriptor cache entries pregenerated. This has a side effect of making wallets large since 6000 pubkeys are written to the wallet by default, instead of the current 2000. scriptPubKeys are kept only in memory and are generated every time a descriptor is loaded. By default, we use the standard BIP 44, 49, 84 derivation paths with an external and internal derivation chain for each.
Descriptors can also be imported with a new `importdescriptors` RPC.
Native descriptor wallets use the `ScriptPubKeyMan` interface introduced in #16341 to add a `DescriptorScriptPubKeyMan`. This defines a different IsMine which uses the simpler model of "does this scriptPubKey exist in this wallet". Furthermore, `DescriptorScriptPubKeyMan` does not have watchonly, so with native descriptor wallets, it is not possible to have a wallet with both watchonly and non-watchonly things. Rather a wallet with `disable_private_keys` needs to be used for watchonly things.
A `--descriptor` option was added to some tests (`wallet_basic.py`, `wallet_encryption.py`, `wallet_keypool.py`, `wallet_keypool_topup.py`, and `wallet_labels.py`) to allow for these tests to use descriptor wallets. Additionally, several RPCs are disabled for descriptor wallets (`importprivkey`, `importpubkey`, `importaddress`, `importmulti`, `addmultisigaddress`, `dumpprivkey`, `dumpwallet`, `importwallet`, and `sethdseed`).
ACKs for top commit:
Sjors:
utACK 223588b1bb (rebased, nits addressed)
jonatack:
Code review re-ACK 223588b1bb.
fjahr:
re-ACK 223588b1bb
instagibbs:
light re-ACK 223588b
meshcollider:
Code review ACK 223588b1bb
Tree-SHA512: 59bc52aeddbb769ed5f420d5d240d8137847ac821b588eb616b34461253510c1717d6a70bab8765631738747336ae06f45ba39603ccd17f483843e5ed9a90986
faec063887 log: Use Join() helper when listing log categories (MarcoFalke)
Pull request description:
This removes the global `ListLogCategories` and replaces it with a one-line member function `LogCategoriesString`, which just calls `Join`.
Should be a straightforward refactor to get rid of a few LOC.
ACKs for top commit:
laanwj:
ACK faec063887
promag:
ACK faec063887, I also think it's fine as it is (re https://github.com/bitcoin/bitcoin/pull/18669#discussion_r412944724).
Tree-SHA512: 2f51f9ce1246eda5630015f3a869e36953c7eb34f311baad576b92d7829e4e88051c6189436271cd0a13732a49698506345b446b98fd28e58edfb5b62169f1c9
fd8e99da57 tests: Add fuzzing harness for functions in primitives/transaction.h (practicalswift)
d5a31b7cb4 tests: Add fuzzing harness for functions in primitives/block.h (practicalswift)
Pull request description:
Add fuzzing harnesses for various classes/functions in `primitives/`.
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: ed54bd5b37ff5e40cfa8d3cd8c65d91a2f64fca87b6a5c3b8ddd6becd876ed172735fb53da4d00a86f318fb94517afd179e07cb28a43edf301ffe4dad703cca4
e9ea95a30d [net processing] Move all const declarations to top of net_processing.cpp (John Newbery)
507b36dd1b [validation] Move all const declarations to top of validation.h (John Newbery)
0109622b08 [validation] Move validation-only consts to validation.cpp (John Newbery)
b8580cacc7 [net processing] Move net processing consts to net_processing.cpp (John Newbery)
Pull request description:
Following the main.cpp split, there are still some constants in the wrong places, eg net_processing constants in validation.h. Move them all to their rightful homes. At the same time, make them constexpr.
Also move all const declarations to the top of their files, and ensure that they all have doxygen comments.
ACKs for top commit:
practicalswift:
ACK e9ea95a30d -- patch looks correct
MarcoFalke:
ACK e9ea95a30d🚉
Tree-SHA512: 44d81da73c7be01e1d36b939789d793f297d3b94f84ea4e7ac853c621cc7054b5a05c7c9e7b83db506db44c16f344541be8f240d955694211e53a84c32b0d2c5
Since the mempool unbroadcast mechanism handles the reattempts for initial
broadcast, the wallet rebroadcast attempts can be much less frequent
(previously ~1/30 min)
- Mempool tracks locally submitted transactions (wallet or rpc)
- Transactions are removed from set when the node receives a GETDATA request
from a peer, or if the transaction is removed from the mempool.
When a CWallet doesn't have a ScriptPubKeyMan for the requested type
in GetNewDestination, give a meaningful error. Also handle this in
Qt which did not do anything with errors.
