While limitations on the influence of attackers on addrman already
exist (affected buckets are restricted to a subset based on incoming
IP / network group), there is no reason to permit them to let them
feed us addresses at more than a multiple of the normal network
rate.
This commit introduces a "token bucket" rate limiter for the
processing of addresses in incoming ADDR and ADDRV2 messages.
Every connection gets an associated token bucket. Processing an
address in an ADDR or ADDRV2 message from non-whitelisted peers
consumes a token from the bucket. If the bucket is empty, the
address is ignored (it is not forwarded or processed). The token
counter increases at a rate of 0.1 tokens per second, and will
accrue up to a maximum of 1000 tokens (the maximum we accept in a
single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
immediately gets 1000 additional tokens, as we actively desire many
addresses from such peers (this may temporarily cause the token
count to exceed 1000).
The rate limit of 0.1 addr/s was chosen based on observation of
honest nodes on the network. Activity in general from most nodes
is either 0, or up to a maximum around 0.025 addr/s for recent
Bitcoin Core nodes. A few (self-identified, through subver) crawler
nodes occasionally exceed 0.1 addr/s.
If a ScriptPubKeyMan does not implement Upgrade, then using upgraewallet
will fail unexpectedly. By changing the default to return true, then
this error can be avoided. This is still correct because a successful
upgrade can be that nothing happened.
7593b06bd1 test: ensure I2P addresses are relayed (Vasil Dimov)
e7468139a1 test: make CAddress in functional tests comparable (Vasil Dimov)
33e211d2a4 test: implement ser/unser of I2P addresses in functional tests (Vasil Dimov)
86742811ce test: use NODE_* constants instead of magic numbers (Vasil Dimov)
ba45f02708 net: relay I2P addresses even if not reachable (by us) (Vasil Dimov)
Pull request description:
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
https://github.com/bitcoin/bitcoin/pull/20119 before anybody could
connect to I2P because then, for sure, it would have been useless.
Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
ACKs for top commit:
jonatack:
ACK 7593b06bd1
naumenkogs:
ACK 7593b06bd1.
laanwj:
Code review ACK 7593b06bd1
kristapsk:
ACK 7593b06bd1. Code looks correct, tested that functional test suite passes and also that `test/functional/p2p_addrv2_replay.py` fails if I undo changes in `IsRelayable()`.
Tree-SHA512: c9feec4a9546cc06bc2fec6d74f999a3c0abd3d15b7c421c21fcf2d610eb94611489e33d61bdcd5a4f42041a6d84aa892f7ae293b0d4f755309a8560b113b735
b1d905c225 p2p: earlier continuation when no remaining eviction candidates (Vasil Dimov)
c9e8d8f9b1 p2p: process more candidates per protection iteration (Jon Atack)
02e411ec45 p2p: iterate eviction protection only on networks having candidates (Jon Atack)
5adb064574 bench: add peer eviction protection benchmarks (Jon Atack)
566357f8f7 refactor: move GetRandomNodeEvictionCandidates() to test utilities (Jon Atack)
Pull request description:
This follow-up to #21261 improves `ProtectEvictionCandidatesByRatio()` for better performance.
Benchmarks are added; the performance improvement is between 2x and 5x for the benchmarked cases (CPU 2.50GHz, Turbo off, performance mode, Debian Clang 11 non-debug build).
```
$ ./src/bench/bench_bitcoin -filter="EvictionProtection*.*"
```
The refactored code is well-covered by existing unit tests and also a fuzzer.
- `$ ./src/test/test_bitcoin -t net_peer_eviction_tests`
- `$ FUZZ=node_eviction ./src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/node_eviction`
ACKs for top commit:
klementtan:
Tested and code review ACK b1d905c2.
vasild:
ACK b1d905c225
jarolrod:
ACK b1d905c225
Tree-SHA512: a3a6607b9ea2fec138da9780c03f63e177b6712091c5a3ddc3804b896a7585216446310280791f5e20cc023d02d2f03a4139237e12b5c1d7f2a1fa1011610e96
Negative `nNew` or `nTried` are not possible during normal operation.
So, if we read such values during unserialize, report addrman
corruption.
Fixes https://github.com/bitcoin/bitcoin/issues/22450
ceb7b35a39 refactor: move UpdateTip into CChainState (James O'Beirne)
4abf0779d6 refactor: no mempool arg to GetCoinsCacheSizeState (James O'Beirne)
46e3efd1e4 refactor: move UpdateMempoolForReorg into CChainState (James O'Beirne)
617661703a validation: make CChainState::m_mempool optional (James O'Beirne)
Pull request description:
Make `CChainState::m_mempool` optional by making it a pointer instead of a reference. This will allow a simplification to assumeutxo semantics (see https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905) and help facilitate the `-nomempool` option.
ACKs for top commit:
jnewbery:
ACK ceb7b35a39
naumenkogs:
ACK ceb7b35a39
ryanofsky:
Code review ACK ceb7b35a39 (just minor style and test tweaks since last review)
lsilva01:
Code review ACK and tested on Signet ACK ceb7b35a39
MarcoFalke:
review ACK ceb7b35a39😌
Tree-SHA512: cc445ad33439d5918cacf80a6354eea8f3d33bb7719573ed5b970fad1a0dab410bcd70be44c862b8aba1b71263b82d79876688c553e339362653dfb3d8ec81e6
fa5658ed07 Use DeploymentEnabled to hide VB deployments (MarcoFalke)
fa11fecf0d doc: Move buried deployment doc to the enum that enumerates them (MarcoFalke)
Pull request description:
Plus a doc commit.
ACKs for top commit:
jnewbery:
utACK fa5658ed07
ajtowns:
utACK fa5658ed07
Tree-SHA512: 2aeceee0674feb603d76656eff40695b7d7305de309f837bbb6a8c1dbb1d0b962b741f06ab7b9a8b1dbd1964c9c0c9aa5dc9588fd8e6d896e620b69e08eedbaa
be8d9c262f Merge bitcoin-core/secp256k1#965: gen_context: Don't use any ASM
aeece44599 gen_context: Don't use any ASM
7688a4f13a Merge bitcoin-core/secp256k1#963: "Schnorrsig API overhaul" fixups
90e83449b2 ci: Add C++ test
f698caaff6 Use unsigned char consistently for byte arrays
b5b8e7b719 Don't declare constants twice
769528f307 Don't use string literals for char arrays without NUL termination
2cc3cfa583 Fix -Wmissing-braces warning in clang
0440945fb5 Merge #844: schnorrsig API overhaul
ec3aaa5014 Merge #960: tests_exhaustive: check the result of secp256k1_ecdsa_sign
a1ee83c654 tests_exhaustive: check the result of secp256k1_ecdsa_sign
253f90cdeb Merge bitcoin-core/secp256k1#951: configure: replace AC_PATH_PROG to AC_CHECK_PROG
446d28d9de Merge bitcoin-core/secp256k1#944: Various improvements related to CFLAGS
0302138f75 ci: Make compiler warning into errors on CI
b924e1e605 build: Ensure that configure's compile checks default to -O2
7939cd571c build: List *CPPFLAGS before *CFLAGS like on the compiler command line
595e8a35d8 build: Enable -Wcast-align=strict warning
07256267ff build: Use own variable SECP_CFLAGS instead of touching user CFLAGS
4866178dfc Merge bitcoin-core/secp256k1#955: Add random field multiply/square tests
75ce488c2a Merge bitcoin-core/secp256k1#959: tests: really test the non-var scalar inverse
41ed13942b tests: really test the non-var scalar inverse
5f6ceafcfa schnorrsig: allow setting MSGLEN != 32 in benchmark
fdd06b7967 schnorrsig: add tests for sign_custom and varlen msg verification
d8d806aaf3 schnorrsig: add extra parameter struct for sign_custom
a0c3fc177f schnorrsig: allow signing and verification of variable length msgs
5a8e4991ad Add secp256k1_tagged_sha256 as defined in BIP-340
b6c0b72fb0 schnorrsig: remove noncefp args from sign; add sign_custom function
bdf19f105c Add random field multiply/square tests
8ae56e33e7 Merge #879: Avoid passing out-of-bound pointers to 0-size memcpy
a4642fa15e configure: replace AC_PATH_PROG to AC_CHECK_PROG
1758a92ffd Merge #950: ci: Add ppc64le build
c58c4ea470 ci: Add ppc64le build
7973576f6e Merge #662: Add ecmult_gen, ecmult_const and ecmult to benchmark
8f879c2887 Fix array size in bench_ecmult
2fe1b50df1 Add ecmult_gen, ecmult_const and ecmult to benchmark
593e6bad9c Clean up ecmult_bench to make space for more benchmarks
50f3367712 Merge #947: ci: Run PRs on merge result even for i686
a35fdd3478 ci: Run PRs on merge result even for i686
442cee5baf schnorrsig: add algolen argument to nonce_function_hardened
df3bfa12c3 schnorrsig: clarify result of calling nonce_function_bip340 without data
99e8614812 README: mention schnorrsig module
3dc8c072b6 Merge #846: ci: Run ASan/LSan and reorganize sanitizer and Valgrind jobs
02dcea1ad9 ci: Make test iterations configurable and tweak for sanitizer builds
489ff5c20a tests: Treat empty SECP2561_TEST_ITERS as if it was unset
fcfcb97e74 ci: Simplify to use generic wrapper for QEMU, Valgrind, etc
de4157f13a ci: Run ASan/LSan and reorganize sanitizer and Valgrind jobs
399722a63a Merge #941: Clean up git tree
09b3bb8648 Clean up git tree
bf0ac46066 Merge #930: Add ARM32/ARM64 CI
202a030f7d Merge #850: add `secp256k1_ec_pubkey_cmp` method
1e78c18d5b Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards
69394879b6 Merge #926: secp256k1.h: clarify that by default arguments must be != NULL
6eceec6d56 add `secp256k1_xonly_pubkey_cmp` method
0d9561ae87 add `secp256k1_ec_pubkey_cmp` method
22a9ea154a contrib: Explain explicit header guards
6c52ae8724 Merge #937: Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs.
185a6af227 Merge #925: changed include statements without prefix 'include/'
14c9739a1f tests: Improve secp256k1_ge_set_all_gej_var for some infinity inputs
4a19668c37 tests: Test secp256k1_ge_set_all_gej_var for all infinity inputs
3c90bdda95 change local lib headers to be relative for those pointing at "include/" dir
45b6468d7e Have secp256k1_ge_set_all_gej_var initialize all fields. Previous behaviour would not initialize r->y values in the case where infinity is passed in. Furthermore, the previous behaviour wouldn't initialize anything in the case where all inputs were infinity.
31c0f6de41 Have secp256k1_gej_double_var initialize all fields. Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
dd6c3de322 Have secp256k1_ge_set_gej_var initialize all fields. Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
d0bd2693e3 Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM
8bbad7a18e Add asm build to ARM32 CI
7d65ed5214 Add ARM32/ARM64 CI
c8483520c9 Makefile.am: Don't pass a variable twice
2161f31785 Makefile.am: Honor config when building gen_context
99f47c20ec gen_context: Don't use external ASM because it complicates the build
98e0358d29 Merge #933: Avoids a missing brace warning in schnorrsig/tests_impl.h on old compilers
99e2d5be0d Avoids a missing brace warning in schnorrsig/tests_impl.h on old compilers.
34388af6b6 Merge #922: Add mingw32-w64/wine CI build
7012a188e6 Merge #928: Define SECP256K1_BUILD in secp256k1.c directly.
ed5a199bed tests: fopen /dev/urandom in binary mode
ae9e648526 Define SECP256K1_BUILD in secp256k1.c directly.
4dc37bf81b Add mingw32-w64/wine CI build
0881633dfd secp256k1.h: clarify that by default arguments must be != NULL
9570f674cc Avoid passing out-of-bound pointers to 0-size memcpy
git-subtree-dir: src/secp256k1
git-subtree-split: be8d9c262f46309d9b4165b0498b71d704aba8fe
Moved implementations of `ConsumeTxMemPoolEntry`, `ContainsSpentInput`, `ConsumeNetAddr`, and the methods(open, read, write, seek, close) of FuzzedFileProvider from test/fuzz/util.h to test/fuzz/util.cpp.
Since we now have multiple chainstate objects, only one of them is active at any given
time. An active chainstate has a mempool, but there's no point to others having one.
This change will simplify proposed assumeutxo semantics. See the discussion here:
https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
4101ec9d2e doc: mention that we enforce port=0 in I2P (Vasil Dimov)
e0a2b390c1 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov)
41cda9d075 test: ensure I2P ports are handled as expected (Vasil Dimov)
4f432bd738 net: do not connect to I2P hosts on port!=0 (Vasil Dimov)
1f096f091e net: distinguish default port per network (Vasil Dimov)
aeac3bce3e net: change I2P seeds' ports to 0 (Vasil Dimov)
38f900290c net: change assumed I2P port to 0 (Vasil Dimov)
Pull request description:
_This is an alternative to https://github.com/bitcoin/bitcoin/pull/21514, inspired by https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-815049933. They are mutually exclusive. Just one of them should be merged._
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-816564719).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
* Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.
* Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening".
Fixes#21389
ACKs for top commit:
laanwj:
Code review ACK 4101ec9d2e
jonatack:
re-ACK 4101ec9d2e per `git range-diff efff9c3 0b0ee03 4101ec9`, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe
2feec3ce31 net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)
Pull request description:
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
ACKs for top commit:
laanwj:
Code review ACK 2feec3ce31
jonatack:
utACK 2feec3ce31 per `git diff a004833 2feec3c`
hebasto:
ACK 2feec3ce31, tested on Linux Mint 20.1 (x86_64):
Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
This commit fixes some slightly unexpected behaviour when:
- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
as the mempool transaction but a different witness (the "new tx")
Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).
Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.
Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.
Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).
Fix by only adding the transaction to the broadcast set when it's added
to the mempool.
b7a8cd9963 [test] submit same txid different wtxid as mempool tx (glozow)
fdb48163bf [validation] distinguish same txid different wtxid in mempool (glozow)
Pull request description:
On master, if you submit a transaction with the same txid but different witness to the mempool, it thinks the transactions are the same. Users submitting through `BroadcastTransaction()` (i.e. `sendrawtransaction` or the wallet) don't get notified that there's a different transaction in the mempool, although it doesn't crash. Users submitting through `testmempoolaccept()` will get a "txn-already-in-mempool" error.
This PR simply distinguishes between `txn-already-in-mempool` and `txn-same-nonwitness-data-in-mempool`, without handling them differently: `sendrawtransaction` still will not throw, but `testmempoolaccept` will give you a different error.
I believe the intention of #19645 is to allow full swaps of transactions that have different witnesses but identical nonwitness data. Returning a different error message + adding a test was suggested: https://github.com/bitcoin/bitcoin/pull/19645#issuecomment-705109193 so this is that PR.
ACKs for top commit:
naumenkogs:
ACK b7a8cd9963
jnewbery:
Code review ACK b7a8cd9963
theStack:
Code-review ACK b7a8cd9963
darosior:
re-utACK b7a8cd9963
Tree-SHA512: 9c6591edaf8727ba5b4675977adb8cbdef7288584003b6cd659828032dc92d2ae915800a8ef8b6fdffe112c1b660df72297a3dcf2e2e3e1f959c6cb3678c63ee
This is a temporary change to convert I2P addresses that have propagated
with port 8333 to ones with port 0.
It would cause a problem some day if indeed some bitcoin software is
listening on port 8333 only and rejects connections to port 0 and we are
still using SAM 3.1 which only supports port 0. In this case we would
replace 8333 with 0 and try to connect to such nodes.
This commit should be included in 22.0 and be reverted before 23.0 is
released.
When connecting to an I2P host we don't specify destination port and it
is being forced to 0 by the SAM 3.1 proxy, so if we connect to the same
host on two different ports, that would be actually two connections to
the same service (listening on port 0).
Fixes https://github.com/bitcoin/bitcoin/issues/21389
* When accepting an I2P connection, assume the peer has port 0 instead
of the default 8333 (for mainnet). It is not being sent to us, so we
must assume something.
* When deriving our own I2P listen CService use port 0 instead of the
default 8333 (for mainnet). So that we later advertise it to peers
with port 0.
In the I2P protocol SAM 3.1 and older (we use 3.1) ports are not used,
so they are irrelevant. However in SAM 3.2 and newer ports are used and
from the point of view of SAM 3.2, a peer using SAM 3.1 seems to have
specified port=0.
4e44f5bac4 test: Correct outstanding -Werror=sign-compare errors (Ben Woosley)
Pull request description:
I'm unclear on why these aren't failing on CI, but they failed for me locally, e.g.:
```
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned int, int>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned int, int>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned int, int>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:153:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/streams_tests.cpp:122:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned int, int>' requested here
BOOST_CHECK_EQUAL(varint, 54321);
^
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned long long' and 'const long' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned long long, long>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned long long, long>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned long long, long>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:161:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/serfloat_tests.cpp:41:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned long long, long>' requested here
BOOST_CHECK_EQUAL(TestDouble(std::numeric_limits<double>::infinity()), 0x7ff0000000000000);
^
ACKs for top commit:
theStack:
ACK 4e44f5bac4
Tree-SHA512: 8d9e5245676c61207ceacdf78c78a78ccc9fd2a2551d4d8df023513795591334aa2f5e1f4a2a8ed2bfeb381f1e226b6ba84c07e0de29a1f3f00da71f3a257bc1
5b4703c6a7 guix: Test security-check sanity before performing them (Carl Dong)
6cf3345297 scripts: adjust test-symbol-check for guix release environment (fanquake)
1946b5f77c scripts: more robustly test macOS symbol checks (fanquake)
a8127b34bc build: Use and test PE binutils with --reloc-section (Carl Dong)
678348db51 guix: Patch binutils to add security-related disable flags (Carl Dong)
9fdc8afe11 devtools: Improve *-check.py tool detection (Carl Dong)
bda62eab38 ci: skip running the Linux test-security-check target for now (fanquake)
d6ef3543ae lint: Run mypy with --show-error-codes (Carl Dong)
Pull request description:
This is #20980 rebased (to include the Boost Process fix), and with an additional commit (892d6897f1e613084aa0517a660eab2412308e6e) to fix running the `test-security-check` target for the macOS build. It should pass inside Guix, as well as when cross-compiling on Ubuntu, or building natively on macOS.
Note that the `test-security-check` may output some warnings (similar too):
```bash
ld: warning: passed two min versions (10.14, 11.4) for platform macOS. Using 11.4.
ld: warning: passed two min versions (10.14, 11.4) for platform macOS. Using 11.4.
ld: warning: passed two min versions (10.14, 10.14) for platform macOS. Using 10.14.
```
but those can be ignored, and come about due to us passing `-platform_version` when `-mmacosx-version-min` is already part of `CC`.
Guix builds:
```bash
71ed0c7a13a4726300779ffc87f7d271086a2744c36896fe6dc51fe3dc33df2e guix-build-5b4703c6a70d/output/aarch64-linux-gnu/SHA256SUMS.part
9273980a17052c8ec45b77579781c14ab5d189fa25aa29907d5115513dd302b1 guix-build-5b4703c6a70d/output/aarch64-linux-gnu/bitcoin-5b4703c6a70d-aarch64-linux-gnu-debug.tar.gz
9c042179af43c8896eb95a34294df15d4910308dcdba40b2010cd36e192938b8 guix-build-5b4703c6a70d/output/aarch64-linux-gnu/bitcoin-5b4703c6a70d-aarch64-linux-gnu.tar.gz
1ceddecac113f50a952ba6a201cdcdb722e3dc804e663f219bfac8268ce42bf0 guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/SHA256SUMS.part
759597c4e925e75db4a2381c06cda9b9f4e4674c23436148676b31c9be05c7aa guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/bitcoin-5b4703c6a70d-arm-linux-gnueabihf-debug.tar.gz
34e3b6beabaf8c95d7c2ca0d2c3ac4411766694ef43e00bd9783badbbaf045a7 guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/bitcoin-5b4703c6a70d-arm-linux-gnueabihf.tar.gz
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-5b4703c6a70d/output/dist-archive/SKIPATTEST.TAG
3664f6ceee7898caa374281fd877a7597fe491fa2e9f0c174c28d889d60b559c guix-build-5b4703c6a70d/output/dist-archive/bitcoin-5b4703c6a70d.tar.gz
d6bc35ba0750c1440bb32831b8c12cddee62f6dce10fec2650897444c2bf4748 guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/SHA256SUMS.part
a836edf6474ba0c16c19bb217549bac7936c1b44306ed512df58f607ee5568f2 guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/bitcoin-5b4703c6a70d-powerpc64-linux-gnu-debug.tar.gz
7cc91c6805d5069ca3bd1771e77d95f83eb184b137198cbf84d1d11d0a5c5afe guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/bitcoin-5b4703c6a70d-powerpc64-linux-gnu.tar.gz
93b4cb7b83c4975120ad5de5a92f050f5760a2a3f2c37c204c647f5a581c924a guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/SHA256SUMS.part
2266e2c5d0dafa28c6c057ccfc1c439baeab1d714d8c3f64a83015d2827116d2 guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/bitcoin-5b4703c6a70d-powerpc64le-linux-gnu-debug.tar.gz
85f41f42c319b83d049d6fd2e2278c07b40a1e28a2eac596427822c0eef9dc3f guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/bitcoin-5b4703c6a70d-powerpc64le-linux-gnu.tar.gz
1499ca9119926083d8c3714ca10d8d4c8d864cbeee8848fd8445b7a1d081222d guix-build-5b4703c6a70d/output/riscv64-linux-gnu/SHA256SUMS.part
1995fc1a2e45c49d4b0718aff5dcdac931917e8ae9e762fd23f1126abcecc248 guix-build-5b4703c6a70d/output/riscv64-linux-gnu/bitcoin-5b4703c6a70d-riscv64-linux-gnu-debug.tar.gz
266889eb58429a470f0fd7bb123f2ae09b0aef86c47b0390938b3634a8f748a9 guix-build-5b4703c6a70d/output/riscv64-linux-gnu/bitcoin-5b4703c6a70d-riscv64-linux-gnu.tar.gz
cdc3a0dcf80b110443dac5ddf8bc951001a776a651c898c5ea49bb2d487bfe29 guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/SHA256SUMS.part
8538d1eab96c97866b24546c453d95822f24cf9c6638b42ba523eb7aa441cb26 guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx-unsigned.dmg
d1b73133f1da68586b07292a8425f7f851e93f599c016376f23728c041cf39cc guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx-unsigned.tar.gz
5ad94c5f8a5f29405955ff3ab35d137de1acc04398d6c8298fb187b57a6e316a guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx64.tar.gz
8c6d7b3f847faa7b4d16ceecf228f26f146ea982615c1d7a00c57f9230a0c484 guix-build-5b4703c6a70d/output/x86_64-linux-gnu/SHA256SUMS.part
d0a8c99750319ad8046cfa132a54e5c13a08351f94439ae9af0f8e5486c2c2ea guix-build-5b4703c6a70d/output/x86_64-linux-gnu/bitcoin-5b4703c6a70d-x86_64-linux-gnu-debug.tar.gz
d816bb26dd4b0e309f2f576b1cccc6d78743fb2f357daad2da09bb1177330971 guix-build-5b4703c6a70d/output/x86_64-linux-gnu/bitcoin-5b4703c6a70d-x86_64-linux-gnu.tar.gz
65caaa7f648c7eab1eb82c3331a2ca25b8cd4fe41439de55604501e02571de55 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/SHA256SUMS.part
5bf6f7328cbceb0db22a2d7babb07b60cb6dcc19a6db84a1698589b7f5173a06 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win-unsigned.tar.gz
7aabcb56115decef78d3797840b6e49dbc9b202d56f892490e92616fb06fec9e guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64-debug.zip
2f369694648ff9dc5ca1261a1e5874b1c7408ccf2802f9caef56c1334e8a5b7c guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64-setup-unsigned.exe
1c1f92513c4aad38419ff49a7b80bf10e6b1eca01ee8c5e3b2acd1768cf1e3d5 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64.zip
```
ACKs for top commit:
hebasto:
Approach ACK 5b4703c6a7.
Tree-SHA512: 2cd92a245ea64ef7176cf402a1fa5348a9421c30a4d30d01c950c48f6dcc15cf22ce69ffe1657be97e5fccc14bd933d64683c4439b695528ce3dc34d72dda927
00b875ba94 addrman: remove invalid addresses when unserializing (Vasil Dimov)
bdb62096f0 fuzz: reduce possible networks check (Vasil Dimov)
a164cd3ba6 net: simplify CNetAddr::IsRoutable() (Vasil Dimov)
Pull request description:
* Simplify some code, now that we know `CNetAddr::IsRFC4193()` and `CNetAddr::IsTor()` cannot be `true` at the same time.
* Drop Tor v2 addresses when loading addrman from `peers.dat` - they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.
ACKs for top commit:
sipa:
ACK 00b875ba94. Reviewed the code, and tested with -DDEBUG_ADDRMAN (unit tests + mainnet run with peers.dat that contained v2 onions).
laanwj:
Code review and lightly tested ACK 00b875ba94
jonatack:
ACK 00b875ba94 reviewed, debug-built with -DEBUG_ADDRMAN rebased to current master, restarted node on mainnet/signet/testnet and verified that on each chain -addrinfo shows no change in address counts (as expected). Added some sanity check asserts, rebuilt/re-ran test. Checked that the new test fails on master with "test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]"
jarolrod:
ACK 00b875ba94
Tree-SHA512: 6ed8e6745134b1b94fffaba28482de909ea39483b46b7f57bda61cdbae7a51251d15cb674de3631772fbeabe153d77a19269f96e62a89102a2d5c01e48f0ba06
in ProtectEvictionCandidatesByRatio().
With this change, `if (n.count == 0) continue;` will be true
if a network had candidates protected in the first iterations
and has no candidates remaining to be protected in later iterations.
Co-authored-by: Jon Atack <jon@atack.com>
for the usual case when some of the protected networks
don't have eviction candidates, to reduce the number
of iterations in ProtectEvictionCandidatesByRatio().
Picks up an idea in ef411cd2 that I had dropped.
in ProtectEvictionCandidatesByRatio().
Thank you to Vasil Dimov, whose suggestions during a post-merge
discussion about PR 21261 reminded me that I had done this in
earlier versions of the PR, e.g. commits like ef411cd2.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
This is important to make sure that we're not testing tools different
from the one we're building with.
Introduce determine_wellknown_cmd, which encapsulates how we
should handle well-known tools specification (IFS splitting, env
override, etc.).
797b3ed909 script: remove gitian reference from symbol-check.py (fanquake)
15fc9a0299 guix: add additional documentation to patches (fanquake)
4516e5ec92 lint: exclude Guix patches from spell-checking (fanquake)
de6ca41a52 guix: no-longer pass --enable-glibc-back-compat to Guix (fanquake)
84dd81fb5b build: remove glibc backcompat requirement for Linux symbol checks (fanquake)
Pull request description:
Now that our Guix toolchains are based on glibc 2.24 and 2.27 (RISCV), we don't need to use the `--enable-glibc-back-compat` option to produce binaries that don't use any symbols from glibc 2.17 and 2.27 or later.
This also adds additional documentation to some Guix patches (pointed out in #22365) and removes Guix patches from the spelling linter, because that isn't our spelling.
Symbol usage: https://gist.github.com/fanquake/d15604fc580718444c5aa4b3c3c75fdc.
Guix Builds:
```bash
bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
ed54e6a6cf4fab328557c0c72eb08c73f2a58c6c70959544cf4b1882e75ea69e guix-build-797b3ed90900/output/aarch64-linux-gnu/SHA256SUMS.part
83bd9dadc59f89f848d143fa4fc3964f16fe0b4bdf35e5093b577ff2c4bd1f43 guix-build-797b3ed90900/output/aarch64-linux-gnu/bitcoin-797b3ed90900-aarch64-linux-gnu-debug.tar.gz
94cb8c35281f12dec6ea5b390b66cad5e27ac8c45a30c42c8d38c438695d54c0 guix-build-797b3ed90900/output/aarch64-linux-gnu/bitcoin-797b3ed90900-aarch64-linux-gnu.tar.gz
7318b63d65c0aa52d2446de8e1f40658d2e47ab8fb0268820c3b7585d140fb23 guix-build-797b3ed90900/output/arm-linux-gnueabihf/SHA256SUMS.part
95e1ffb372964b73f539653ca703b70cf0c018801a9c4c0ffc46a0b63539253c guix-build-797b3ed90900/output/arm-linux-gnueabihf/bitcoin-797b3ed90900-arm-linux-gnueabihf-debug.tar.gz
039d3842e6499626cf955ae0a7590dd6b3d0935cdc217c98aaf9d156b0ebd3b4 guix-build-797b3ed90900/output/arm-linux-gnueabihf/bitcoin-797b3ed90900-arm-linux-gnueabihf.tar.gz
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-797b3ed90900/output/dist-archive/SKIPATTEST.TAG
2c4e7b6e7aff63ba811e5bf59362d16866c3a358f8844fba8739a61192870622 guix-build-797b3ed90900/output/dist-archive/bitcoin-797b3ed90900.tar.gz
955029b949c368eabd517dd33040d2f01e2ac6a55e7b4f9107907a7c6e0c6060 guix-build-797b3ed90900/output/powerpc64-linux-gnu/SHA256SUMS.part
fd6d6b137f8efedf58a879d11205b1d4649e1f97d7f91e193239ef206fcc285d guix-build-797b3ed90900/output/powerpc64-linux-gnu/bitcoin-797b3ed90900-powerpc64-linux-gnu-debug.tar.gz
51736ac8e77737999f1b5bd4c381b0016f19a8d5e40e786fe941ff04e84c11c9 guix-build-797b3ed90900/output/powerpc64-linux-gnu/bitcoin-797b3ed90900-powerpc64-linux-gnu.tar.gz
8c244c16bfa46c1efdb120e1d91fdd14d3f14eefee8d7e1fbb0a9b4664a5c315 guix-build-797b3ed90900/output/powerpc64le-linux-gnu/SHA256SUMS.part
704ee593251a1b1c65a5bebeef93b23f266af4e8cbf8ae556150c3b2e8f06a6c guix-build-797b3ed90900/output/powerpc64le-linux-gnu/bitcoin-797b3ed90900-powerpc64le-linux-gnu-debug.tar.gz
0ec06ae7d344de20d61e3965d8b383747ef20b0e9d93a3165733ea23bdf2ead8 guix-build-797b3ed90900/output/powerpc64le-linux-gnu/bitcoin-797b3ed90900-powerpc64le-linux-gnu.tar.gz
2dd6c6ecc67b0ea40ca9c43f92efca81ccd054b8db8c197ad84ad9674d510a25 guix-build-797b3ed90900/output/riscv64-linux-gnu/SHA256SUMS.part
5ebb27a855a677f7a188d83995be6b2a3ea8606be152abb7fc7832713fb0677a guix-build-797b3ed90900/output/riscv64-linux-gnu/bitcoin-797b3ed90900-riscv64-linux-gnu-debug.tar.gz
bdaf1783f5e1861597afa37c1880364e118d9a7a7af8017302d82202791019f6 guix-build-797b3ed90900/output/riscv64-linux-gnu/bitcoin-797b3ed90900-riscv64-linux-gnu.tar.gz
726c9092b60ac2e7d7e14b2c24467fcf276a6f89170a871ddab9dce6ac230699 guix-build-797b3ed90900/output/x86_64-apple-darwin18/SHA256SUMS.part
2af4d709b44952654f3c08c86593bf2ccc9a44ed422783a1b95b8a199a894db2 guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx-unsigned.dmg
fd49ba445aa6cf3d8c47019a05e9e5740cb0f53349344dd80671297127f49f1a guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx-unsigned.tar.gz
3f51cbf8cf18420d4be70e656aa993675cf5e828a255c2030047ae2e059ed5b7 guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx64.tar.gz
afd1edee1447bb88d81e972abfae4c4e065b5b1827769f033cff9472084c7c1b guix-build-797b3ed90900/output/x86_64-linux-gnu/SHA256SUMS.part
ec468ef886d25e685f4f7a18b4f7d497dedf757495e0d5beb72c23cc32ab69b5 guix-build-797b3ed90900/output/x86_64-linux-gnu/bitcoin-797b3ed90900-x86_64-linux-gnu-debug.tar.gz
1934d7294f0c9e083d38a3f68d4a61cd679defa79ce0a89f77386978692b9b18 guix-build-797b3ed90900/output/x86_64-linux-gnu/bitcoin-797b3ed90900-x86_64-linux-gnu.tar.gz
94c11c328a628052eb6f50e9816aa768f87ea7acfbbbafdab60f6928da766811 guix-build-797b3ed90900/output/x86_64-w64-mingw32/SHA256SUMS.part
fd371922ba93d81bd4a2b711d617af6756f9f0494db6d83aa0e5f491a24168ef guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win-unsigned.tar.gz
4e4ad976bc029bbbf9596ad8493accaaba8b0d5c598dd342f8da330609bbdf21 guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64-debug.zip
3a89a16b9101e9a17d98efb9234b5bdd264c0bba2c6326511017730e1a08311f guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64-setup-unsigned.exe
e285ab737e3c843fd3f1c26c2f053e421a3c39b33995747ce48281884d3f28d1 guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64.zip
```
ACKs for top commit:
sipa:
utACK 797b3ed909
hebasto:
ACK 797b3ed909
Tree-SHA512: 3a569702d8832c155c5ce8d2f6d823f7f12603885576078bc5192bc9038a48261ecb541800f79d1e9bc86d71fa640265c5b8b89df9d8bb680b3bb05d9d78a666
986bf78d7e qt: Emit dataChanged signal to dynamically re-sort Peers table (Hennadii Stepanov)
Pull request description:
[By default](https://doc.qt.io/qt-5/qsortfilterproxymodel.html#details), the `PeerTableSortProxy`
> dynamically re-sorts ... data whenever the original model changes.
That is not the case on master (8cdf91735f) as in ecbd911538 (#164) no signals are emitted to notify about model changes.
This PR uses a dedicated [`dataChanged`](https://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged) signal.
Fixes#367.
An alternative to #374.
ACKs for top commit:
jarolrod:
ACK 986bf78d7e
Tree-SHA512: dcb92c2f9a2c632880429e9528007db426d2ad938c64dfa1f1538c03e4b62620df52ad7daf33b582976c67b472ff76bc0dae707049f4bbbd4941232cee9ce3d4
cd46c11577 qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
9ea1da6fc9 qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov)
Pull request description:
This PR guaranties that the "eye" sign won't be hidden for very long addresses/labels.
No longer need to extend `TransactionOverviewWidget` widget width to make "eye" signs shown:
Fixes https://github.com/bitcoin-core/gui/issues/373
ACKs for top commit:
jarolrod:
ACK cd46c11577
Tree-SHA512: 0602b5bb65d53c5b18e86260750006bba03adbae181917b5a2b7f89b17290bd1f57b4f80adaba32f42cc6fb468598a888b12c0b6b09005d2f2c07bd4d1ad334a
* Assert when a type is missing
* Add missing WitnessV1Taproot
* Limit WitnessUnknown to version [2, 16], to avoid abiguity
* Limit WitnessUnknown to size [2, 40], to avoid invalid sizes
e48826ad87 tests: remove ComputeBlockVersion shortcut from versionbits tests (Anthony Towns)
c5f36725e8 [refactor] Move ComputeBlockVersion into VersionBitsCache (Anthony Towns)
4a69b4dbe0 [move-only] Move ComputeBlockVersion from validation to versionbits (Anthony Towns)
0cfd6c6a8f [refactor] versionbits: make VersionBitsCache a full class (Anthony Towns)
8ee3e0bed5 [refactor] rpc/blockchain.cpp: SoftForkPushBack (Anthony Towns)
92f48f360d deploymentinfo: Add DeploymentName() (Anthony Towns)
ea68b3a572 [move-only] Rename versionbitsinfo to deploymentinfo (Anthony Towns)
c64b2c6a0f scripted-diff: rename versionbitscache (Anthony Towns)
de55304f6e [refactor] Add versionbits deployments to deploymentstatus.h (Anthony Towns)
2b0d291da8 [refactor] Add deploymentstatus.h (Anthony Towns)
eccd736f3d versionbits: Use dedicated lock instead of cs_main (Anthony Towns)
36a4ba0aaa versionbits: correct doxygen comments (Anthony Towns)
Pull request description:
Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from [11398](https://github.com/bitcoin/bitcoin/pull/11398#issuecomment-335599326) "I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing".
This provides three functions: `DeploymentEnabled()` which tests if a deployment can ever be active, `DeploymentActiveAt()` which checks if a deployment should be enforced in the given block, and `DeploymentActiveAfter()` which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments.
This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on `cs_main`. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation.
ACKs for top commit:
jnewbery:
ACK e48826ad87
gruve-p:
ACK e48826ad87
MarcoFalke:
re-ACK e48826ad87🥈
Tree-SHA512: c846ba64436d36f8180046ad551d8b0d9e20509b9bc185aa2639055fc28803dd8ec2d6771ab337e80da0b40009ad959590d5772f84a0bf6199b65190d4155bed
67669ab425 build: Fix Boost Process compatibility with mingw-w64 compiler (Hennadii Stepanov)
Pull request description:
On master (9c3751a0c9) the cross build for Win64 is broken if configured with `--enable-external-signer`:
```
...
CXX crypto/libbitcoin_crypto_base_a-chacha_poly_aead.o
In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23,
from util/system.cpp:9:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:208:51: error: expected ‘)’ before ‘*’ token
208 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_system_query_information_p )(
| ~ ^~
| )
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:223:51: error: expected ‘)’ before ‘*’ token
223 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_query_object_p )(
| ~ ^~
| )
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::NTSTATUS_ boost::process::detail::windows::workaround::nt_system_query_information(boost::process::detail::windows::workaround::SYSTEM_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:239:12: error: ‘nt_system_query_information_p’ does not name a type; did you mean ‘nt_system_query_information’?
239 | static nt_system_query_information_p f = reinterpret_cast<nt_system_query_information_p>(::boost::winapi::get_proc_address(h, "NtQuerySystemInformation"));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| nt_system_query_information
In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23,
from util/system.cpp:9:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:241:14: error: ‘f’ was not declared in this scope
241 | return (*f)(SystemInformationClass, SystemInformation, SystemInformationLength, ReturnLength);
| ^
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::BOOL_ boost::process::detail::windows::workaround::nt_query_object(boost::winapi::HANDLE_, boost::process::detail::windows::workaround::OBJECT_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:253:12: error: ‘nt_query_object_p’ does not name a type; did you mean ‘nt_query_object’?
253 | static nt_query_object_p f = reinterpret_cast<nt_query_object_p>(::boost::winapi::get_proc_address(h, "NtQueryObject"));
| ^~~~~~~~~~~~~~~~~
| nt_query_object
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:255:14: error: ‘f’ was not declared in this scope
255 | return (*f)(Handle, ObjectInformationClass, ObjectInformation, ObjectInformationLength, ReturnLength);
| ^
make[2]: *** [Makefile:9906: util/libbitcoin_util_a-system.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CXX crypto/libbitcoin_crypto_base_a-chacha20.o
make[2]: Leaving directory '/home/hebasto/GitHub/bitcoin/src'
make[1]: *** [Makefile:16141: all-recursive] Error 1
make[1]: Leaving directory '/home/hebasto/GitHub/bitcoin/src'
make: *** [Makefile:820: all-recursive] Error 1
```
The upstream bug: https://github.com/boostorg/process/issues/96
Also see: https://stackoverflow.com/a/59338759https://github.com/bitcoin/bitcoin/pull/22348#issuecomment-871061160:
> [This commit](7fc41b2815), containing the `__kernel_entry` [SAL annotations](https://docs.microsoft.com/en-us/cpp/code-quality/using-sal-annotations-to-reduce-c-cpp-code-defects?view=msvc-160) was included in Boost Process as part of the `1.71.0` release, which broke support for compiling with mingw-w64 because it doesn't define the `__kernel_entry` SAL annotation (but it does define some others, i.e see [`sal.h`](https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sal.h)).
>
> A [commit was made](d7a721ee0d) to remove the annotations, however, it hasn't made it into either of the two Boost releases that have happened since (1.75.0 & 1.76.0). Meaning that this is currently needed for all versions of Boost process from 1.71.0 onwards.
ACKs for top commit:
fanquake:
ACK 67669ab425 - thanks for updating this.
Tree-SHA512: 5931ca1fb77ce38c042cf5a7556add024ea2386c208bf26c792a8ca4a771d97fac9802c32fa8aa2e3de1ad35f3362d8c066f0a83ee675859d226c602fd0bcf93
6084d2caed wallet: do not spam about non-existent spk managers (S3RK)
Pull request description:
Avoid spam in logs during `loadwallet`, `listdescriptors` and probably other commands as well.
**`loadwallet` Before:**
```
2021-06-24T06:31:45Z init message: Loading wallet…
2021-06-24T06:31:45Z [desc] Wallet File Version = 169900
2021-06-24T06:31:45Z [desc] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Wallet completed loading in 197ms
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] setKeyPool.size() = 0
2021-06-24T06:31:45Z [desc] mapWallet.size() = 0
2021-06-24T06:31:45Z [desc] m_address_book.size() = 0
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
{
"name": "desc",
"warning": ""
}
```
**After:**
```
2021-06-24T06:26:58Z init message: Loading wallet…
2021-06-24T06:26:58Z [desc] Wallet File Version = 169900
2021-06-24T06:26:58Z [desc] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
2021-06-24T06:26:58Z [desc] Wallet completed loading in 158ms
2021-06-24T06:26:58Z [desc] setKeyPool.size() = 0
2021-06-24T06:26:58Z [desc] mapWallet.size() = 0
2021-06-24T06:26:58Z [desc] m_address_book.size() = 0
{
"name": "desc",
"warning": ""
}
```
ACKs for top commit:
achow101:
ACK 6084d2caed
Tree-SHA512: c7d7345c3182a575db088fd731b7f6e428c42e4f3f2e10d5adb50bf74a2defe88768e65ebb91a08590be48cf766a5697e36fafa73f68ffe45e76a60600f072e2
b945a31afa wallet: erase spkmans rather than setting to nullptr (Andrew Chow)
Pull request description:
In many places in ScriptPubKeyMan managing code, we assume that the ScriptPubKeyMan being retrieved actually exists and is not a nullptr. Thus removing a ScriptPubKeyMan requires erasing the object from the map rather than setting it to a nullptr.
This fixes a segmentation fault that can be reached with `test/functional/wallet_descriptors.py --descriptors`
ACKs for top commit:
S3RK:
ACK b945a31
Tree-SHA512: 344a4cf9b1c168428750c751dcd24c52032506f20c81977fe93c4b5307ea209de72bb62a9c5284820f225b03acdc9573fceb734833d29b82f49d5a799ddcaea7
2f23ad2c40 qt: allow prompt icon to be colorized (Jarol Rodriguez)
Pull request description:
Opening the console on macOS, while in dark mode, the console prompt icon will not be colorized white like other icons. This applies the `platformStyle` to the icon so that It can be colorized white.
While here, refactor the `promptIcon` widget from a `QPushButton` to `QLabel`; which is more appropriate, per [Qt Docs](https://doc.qt.io/qt-5/qlabel.html#details):
> QLabel is used for displaying text or an image. No user interaction functionality is provided.
| Master | PR |
| ----------- | ----------- |
|  |  |
ACKs for top commit:
hebasto:
ACK 2f23ad2c40
Tree-SHA512: 21f8b1610e4820c9064bbd08608b5467e5b9499e2a3b149ff223e37b60e7d560497255c733eafa5434628a84b9f7b7c91d8b0f34b02be2f9ceb3ab21a4d555a8
9d5bf6bf01 GUI: Always call parent changeEvent handler (Luke Dashjr)
c901d4d8ce GUI: Enable palette change adaptation on all platforms (Luke Dashjr)
Pull request description:
The changes to support macOS "Dark Mode" are valid for any platform, and should work so long as Qt implements the PaletteChange event. (Worst case, we're no worse off with trying.)
Additionally, we shouldn't block the parent classes from implementing event handlers. Who knows what side effects that could have.
ACKs for top commit:
hebasto:
ACK 9d5bf6bf01, tested on Linux Mint 20.1 (Qt 5.12.8) with the [`qt5ct`](https://packages.ubuntu.com/focal/qt5ct) package installed.
kristapsk:
ACK 9d5bf6bf01. Tested on Gentoo Linux with Xfce4 and Qt 5.15.2, does not break anything on my computer.
Tree-SHA512: dce2fff0ff129eda208132390a37424ff9607539287dbdbfdfd659ed9c4ea0472541e987489a04fd935e391dc006a35bfc9cfa9bcff33602b7dbd29b81c51626
In many places in ScriptPubKeyMan managing code, we assume that the
ScriptPubKeyMan being retrieved actually exists and is not a nullptr.
Thus removing a ScriptPubKeyMan requires erasing the object from the
map rather than setting it to a nullptr.
181181019c refactor: remove m_internal from DescriptorSPKman (S3RK)
Pull request description:
Rationale: improve consistency between `CWallet` and `DescriptorScriptPubKeyMan`; simplify `ScriptPubKeyMan` interface.
Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size).
ACKs for top commit:
instagibbs:
reACK 181181019c
achow101:
reACK 181181019c
Tree-SHA512: d5613b7f6795b290bfa0fd8cb0536de1714d0cf72cba402266bd06d550758ebad690b54fc0a336a1c7414b5814aa4a37c90a6ae89926474a97d30956d7e034ff
3efaf83c75 wallet: deactivate descriptor (S3RK)
6737d9655b test: wallet importdescriptors update existing (S3RK)
586f1d53d6 wallet: maintain SPK consistency on internal flag change (S3RK)
f1b7db1474 wallet: don't mute exceptions in importdescriptors (S3RK)
bf68ebc1cd wallet: allow to import same descriptor twice (S3RK)
Pull request description:
Rationale: allow updating existing descriptors with `importdescriptors` command.
Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error.
With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).
ACKs for top commit:
achow101:
re-ACK 3efaf83c75
meshcollider:
Code review ACK 3efaf83c75
jonatack:
Light ACK 3efaf83c75 per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py
Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843
e6cf0ed92d wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a8 Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8b wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e543 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b83 descriptors: Cache last hardened xpub (Andrew Chow)
cacc391098 Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75c Refactor Cache merging and writing (Andrew Chow)
976b53b085 Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92d
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92d reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92d
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
f9e37f33ce doc: IsFinalTx comment about nSequence & OP_CLTV (Yuval Kogman)
Pull request description:
It's somewhat surprising that a transaction's `nLockTime` field is ignored
when all `nSequence` fields are final, so this change aims to clarify this
behavior and cross reference relevant details of `OP_CHECKLOCKTIMEVERIFY`.
ACKs for top commit:
MarcoFalke:
ACK f9e37f33ce
Tree-SHA512: 88460dacbe4b8115fb1948715f09b21d4f34ba1da9e88d52f0b774a969f845e9eddc5940e7fee66eacdd3062dc40d6d44c3f282b0e5144411fd47eb2320b44f5
Descriptor in itself is neither internal or external.
It's responsibility of a wallet to assign and manage descriptors
for a specific purpose. Duplicating such information could lead to
inconsistencies and unexpected behaviour.
Rename BIP9SoftForkPushBack and BuriedSoftForkPushBack to SoftForkPushBack
and have the compiler figure out which one to use based on the deployment
type. Avoids the need to update the file when burying a deployment.
Adds support for versionbits deployments to DeploymentEnabled,
DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache
from validation to deploymentstatus.
Provides DeploymentEnabled, DeploymentActiveAt, and DeploymentActiveAfter
helpers for checking the status of buried deployments. Can be overloaded
so the same syntax works for non-buried deployments, allowing future
soft forks to be changed from signalled to buried deployments without
having to touch the implementation code.
Replaces IsWitnessEnabled and IsScriptWitnessEnabled.
fa92e60f38 refactor: Make httpserver work queue a unique_ptr (MarcoFalke)
Pull request description:
This simplifies the code a bit because `if (p) { delete p; p = nullptr; }` can be replaced by a call to the `reset()` member.
ACKs for top commit:
promag:
Core review ACK fa92e60f38.
jonatack:
ACK fa92e60f38 code review, debug build clean, ran test/functional/interface*.py tests locally as a sanity check
hebasto:
ACK fa92e60f38, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 6b122162317dd4ad6889341745c7ac1903a3ee510f6548f46dc356308442a6eff13eb8dc604c38ba18783e7a66d2b836d641a8594ff980a010c12c97f3856684
8888cf45f5 Remove unused wallet pointer from NotifyAddressBookChanged (MarcoFalke)
faf3640303 Remove unused wallet pointer from NotifyTransactionChanged signal (MarcoFalke)
Pull request description:
The signals are members of the wallet, so passing the pointer would be redundant even if it was used.
Also, fix `with` -> `without`, which was forgotten in commit ca4cf5cff6.
ACKs for top commit:
jonatack:
Code review ACK 8888cf45f5 also verified with/without lock cs_wallet status for each of the two functions and debian clang 11 debug build clean
promag:
Code review ACK 8888cf45f5.
theStack:
Code review ACK 8888cf45f5
Tree-SHA512: e3b80931ce9bcb05213619f5435ac7c21d3c7848643950a70db610902bd1803c92bb75e501d46b0e519bc576901f160e088e8882c4f1adce892a80df565f897b
fa0d9211ef refactor: Remove chainparams arg from CChainState member functions (MarcoFalke)
fa38947125 refactor: Remove ::Params() global from inside CChainState member functions (MarcoFalke)
Pull request description:
The `::Params()` global is verbose and confusing. Also it makes tests a bit harder to write because they'd have to mock a global.
Fix all issues by simply using a member variable that points to the right params.
(Can be reviewed with `--word-diff-regex=.`)
ACKs for top commit:
jnewbery:
ACK fa0d9211ef
kiminuo:
utACK fa0d9211
theStack:
ACK fa0d9211ef🍉
Tree-SHA512: 44676b19c9ed471ccb536331d3029bad192d7d50f394fd7b8527ec431452aeec8c4494164b9cf8e16e0123c4463b16be864366c6b599370032c17262625a0356
fa9ebedec3 Reject invalid coin height and output index when loading assumeutxo (MarcoFalke)
Pull request description:
It should be impossible to have a coin at a height higher than the height of the snapshot block, so reject those early to avoid integer wraparounds and hash collisions later on.
Same for the outpoint index.
Both issues were found by fuzzing:
* The height issue by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793
* The outpoint issue by my fuzz server: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793#c2
ACKs for top commit:
practicalswift:
cr ACK fa9ebedec3: patch looks correct
jamesob:
crACK fa9ebedec3
theStack:
Code review ACK fa9ebedec3
benthecarman:
crACK fa9ebedec3
Tree-SHA512: dae7caee4b3862b23ebdf2acb7edec4baf75b0dbf1409b370b1a73aa6b632b317ebfac596dcbaf4edfb1301b513f45465ea75328962460f35e2af0d7e547c9ac
.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on FORCE and mark FORCE as phony.
$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch FORCE
$ make print-host
host=x86_64-pc-linux-gnu
Instead of having a large blob of cache merging code in TopUp, refactor
this into DescriptorCache so that it can merge and provide a diff
(another DescriptorCache containing just the items that were added).
Then TopUp can just write everything that was in the diff.
fa34cb8024 cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
Pull request description:
`seconds` is not enough precision to "exactly" store a timestamp n seconds into the future. Improve the precision by using `microseconds`. Fixes#22325
Also, use chrono literals.
ACKs for top commit:
jonatack:
ACK fa34cb8024 review, debug-built, tested
theStack:
Tested ACK fa34cb8024
Tree-SHA512: 7158da8545f9998a82bcc8636e04564efdb1e1be43b4288298c151b4df29ad47a2760259eefadd4a01db92ea18a1e017f3febc1cd8c69a4b28c86180229d8c90
754f134a50 wallet: Add error message to GetReservedDestination (Andrew Chow)
87a0e7a3b7 Disallow bech32m addresses for legacy wallet things (Andrew Chow)
6dbe4d1072 Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests (Andrew Chow)
699dfcd8ad Opportunistically use bech32m change addresses if available (Andrew Chow)
0262536c34 Add OutputType::BECH32M (Andrew Chow)
177c15d2f7 Limit LegacyScriptPubKeyMan address types (Andrew Chow)
Pull request description:
Currently bech32m addresses are classfied as bech32. Because bech32m is incompatible with bech32, we need to define a new `OutputType` for it so that it can be handled correctly. This PR adds `OutputType::BECH32M`, updates all of the relevant `OutputType` classifications, and handle requests for bech32m addresses. There is now a `bech32m` address type string that can be used.
* `tr()` descriptors now report their output type as `OutputType::BECH32M`. `WtinessV1Taproot` and `WitnessUnknown` are also classified as `OutputType::BECH32M`.
* Bech32m addresses are completely disabled for legacy wallets. They cannot be imported (explicitly disallowed in `importaddress` and `importmulti`), will not be created when getting all destinations for a pubkey, and will not be added with `addmultisigaddress`. Additional protections have been added to `LegacyScriptPubKeyMan` to disallow attempting to retrieve bech32m addresses.
* Since Taproot multisigs are not implemented yet, `createmultisig` will also disallow the bech32m address type.
* As Taproot is not yet active, `DescriptorScriptPubKeyMan` cannot and will not create a `tr()` descriptor. Protections have been added to make sure this cannot occur.
* The change address type detection algorithm has been updated to return `bech32m` when there is a segwit v1+ output script and the wallet has a bech32m `ScriptPubKeyMan`, falling back to bech32 if one is not available.
ACKs for top commit:
laanwj:
re-review ACK 754f134a50
Sjors:
re-utACK 754f134: only change is switching to `bech32m` in two `wallet_taproot.py` test cases.
fjahr:
re-ACK 754f134a50
jonatack:
ACK 754f134a50
Tree-SHA512: 6ea90867d3631d0d438e2b08ce6ed930f37d01323224661e8e38f183ea5ee2ab65b5891394a3612c7382a1aff907b457616c6725665a10c320174017b998ca9f
7ad414f4bf doc: add comment about CCoinsViewDBCursor constructor (James O'Beirne)
0f8a5a4dd5 move-only(ish): don't expose CCoinsViewDBCursor (James O'Beirne)
615c1adfb0 refactor: wrap CCoinsViewCursor in unique_ptr (James O'Beirne)
Pull request description:
I tripped over this one for a few hours at the beginning of the week, so I've sort of got a personal vendetta against `CCoinsView::Cursor()` returning a raw pointer.
Specifically in the case of CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction (`Assertion 'dummy_versions_.next_ == &dummy_versions_' failed.`).
This is a pretty simple change.
Related to: https://github.com/bitcoin/bitcoin/issues/21766
See also: https://github.com/google/leveldb/issues/142#issuecomment-414418135
ACKs for top commit:
MarcoFalke:
review ACK 7ad414f4bf🔎
jonatack:
re-ACK 7ad414f4bf modulo suggestion
ryanofsky:
Code review ACK 7ad414f4bf. Two new commits look good and thanks for clarifying constructor comment
Tree-SHA512: 6471d03e2de674d84b1ea0d31e25f433d52aa1aa4996f7b4aab1bd02b6bc340b15e64cc8ea07bbefefa3b5da35384ca5400cc230434e787c30931b8574c672f9
d637a9b397 Taproot descriptor inference (Pieter Wuille)
c7388e5ada Report address as solvable based on inferred descriptor (Pieter Wuille)
29e5dd1a5b consensus refactor: extract ComputeTapleafHash, ComputeTaprootMerkleRoot (Pieter Wuille)
Pull request description:
Includes:
* First commit from #21365, adding TaprootSpendData in SigningProvider
* A refactor to expose ComputeTapleafHash and ComputeTaprootMerkleRoot from script/interpreter
* A tiny change to make `getaddressinfo` report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
* Logic to infer the script tree back from TaprootSpendData, and then use that to infer descriptors.
ACKs for top commit:
achow101:
re-ACK d637a9b397
Sjors:
re-utACK d637a9b
meshcollider:
Code review ACK d637a9b397
Tree-SHA512: 5ab9b95da662382d8549004be4a1297a577d7caca6b068f875c7c9343723931d03fa9cbf133de11f83b74e4851490ce820fb80413c77b9e8495a5f812e505d86
bb719a08db style: remove () from assert in rpc_setban.py (Vasil Dimov)
24b10ebda3 doc: fix grammar in doc/files.md (Vasil Dimov)
dd4e957dcd test: ensure banlist can be read from disk after restart (Vasil Dimov)
d197977ae2 banman: save the banlist in a JSON format on disk (Vasil Dimov)
Pull request description:
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
ACKs for top commit:
jonatack:
Code review re-ACK bb719a08db per `git range-diff 6a67366 4b52c72 bb719a0`
achow101:
Code Review ACK bb719a08db
Tree-SHA512: fc135c3a1fe20bcf5d008ce6bea251b4135e56c78bf8f750b4bd8144c095b81ffe165133cdc7e4715875eec7e7c4e13ad9f5d2450b21102af063d7c8abf716b6
Adds an error output parameter to all GetReservedDestination functions
so that callers can get the actual reason that a change address could
not be fetched. This more closely matches GetNewDestination. This allows
for more granular error messages, such as one that indicates that
bech32m addresses cannot be generated yet.
We don't want the legacy wallet to ever have bech32m addresses so don't
allow importing them. This includes addmultisigaddress as that is a
legacy wallet only RPC
Additionally, bech32m multisigs are not available yet, so disallow them
in createmultisig.
If a transaction as a segwit output, use a bech32m change address if
they are available. If not, fallback to bech32. If bech32 change
addresses are unavailable, fallback to the default address type.
Bech32m addresses need their own OutputType
We are not ready to create DescriptorScriptPubKeyMans which produce
bech32m addresses. So don't allow generating them.
b9e76f1bf0 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7d rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef6 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59 rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf0
promag:
ACK b9e76f1bf0.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not
exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
4e353cb618 http: Release work queue after event base finish (João Barbosa)
Pull request description:
This fixes a race between `http_request_cb` and `StopHTTPServer` where
the work queue is used after release.
Fixes#18856.
ACKs for top commit:
fjahr:
Code review ACK 4e353cb618
achow101:
ACK 4e353cb618
LarryRuane:
ACK 4e353cb618
hebasto:
ACK 4e353cb618, tested (rebased on top of master 9313c4e6aa) on Linux Mint 20.1 (x86_64) using MarcoFalke's [patch](https://github.com/bitcoin/bitcoin/pull/19033#issuecomment-640106647), including different `-rpcthreads`/`-rpcworkqueue` cases. The bug is fixed. The code is correct.
Tree-SHA512: 185d2a9744d0d5134d782bf321ac9958ba17b11a5b3d70b4897c8243e6b146dfd3f23c57aef8e10ae9484374120b64389c1949a9cf0a21dccc47ffc934c20930
30aee2dfe6 tests: Add test for compact block HB selection (Pieter Wuille)
6efbcec4de Protect last outbound HB compact block peer (Suhas Daftuar)
Pull request description:
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
ACKs for top commit:
achow101:
ACK 30aee2dfe6
ariard:
Code ACK 30aee2dfe
jonatack:
ACK 30aee2dfe6
Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
458a345b05 Add support for SIGHASH_DEFAULT in RPCs, and make it default (Pieter Wuille)
c0f0c8eccb tests: check spending of P2TR (Pieter Wuille)
a2380127e9 Basic Taproot signing logic in script/sign.cpp (Pieter Wuille)
49487bc3b6 Make GetInputUTXO safer: verify non-witness UTXO match (Pieter Wuille)
fd3f6890f3 Construct and use PrecomputedTransactionData in PSBT signing (Pieter Wuille)
5cb6502ac5 Construct and use PrecomputedTransactionData in SignTransaction (Pieter Wuille)
5d2e22437b Don't nuke witness data when signing fails (Pieter Wuille)
ce9353164b Permit full precomputation in PrecomputedTransactionData (Pieter Wuille)
e841fb503d Add precomputed txdata support to MutableTransactionSignatureCreator (Pieter Wuille)
a91d532338 Add CKey::SignSchnorr function for BIP 340/341 signing (Pieter Wuille)
e77a2839b5 Use HandleMissingData also in CheckSchnorrSignature (Pieter Wuille)
dbb0ce9fbf Add TaprootSpendData data structure, equivalent to script map for P2[W]SH (Pieter Wuille)
Pull request description:
Builds on top of #22051, adding signing support after derivation support.
Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.
ACKs for top commit:
achow101:
re-ACK 458a345b05
fjahr:
Code review ACK 458a345b05
Sjors:
ACK 458a345b05
Tree-SHA512: 30ed212cf7754763a4a81624ebc084c51727b8322711ac0b390369213c1a891d367ed8b123882ac08c99595320c11ec57ee42304ff22a69afdc3d1a0d55cc711
9550dffa0c fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner)
Pull request description:
This PR is a (quite late) follow-up to #19237 (https://github.com/bitcoin/bitcoin/pull/19237#issuecomment-642203251). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold.
ACKs for top commit:
jamesob:
crACK 9550dffa0c pending CI
Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55
f8866e8c32 Add roundtrip fuzz tests for CAddress serialization (Pieter Wuille)
e2f0548b52 Use addrv2 serialization in anchors.dat (Pieter Wuille)
8cd8f37dfe Introduce well-defined CAddress disk serialization (Pieter Wuille)
Pull request description:
Alternative to #20509.
This makes the `CAddress` disk serialization format well defined, and uses it to enable addrv2 support in anchors.dat (in a way that's compatible with older software). The new format is:
- The first 4 bytes store a format version number. Its low 19 bits are ignored (as those historically stored the `CLIENT_VERSION`), but its high 13 bits specify the actual serialization:
- 0x00000000: LE64 encoding for `nServices`, V1 encoding for `CService` (like pre-BIP155 network serialization).
- 0x20000000: CompactSize encoding for `nServices`, V2 encoding for `CService` (like BIP155 network serialization).
- Any other value triggers an unsupported format error on deserialization, and can be used for future format changes.
- The `ADDRV2_FORMAT` flag in the stream's version does not determine the actual serialization format; it only sets whether or not V2 encoding is permitted.
ACKs for top commit:
achow101:
ACK f8866e8c32
laanwj:
Code review ACK f8866e8c32
vasild:
ACK f8866e8c32
jonatack:
ACK f8866e8c32 tested rebased to master and built/run/restarted with DEBUG_ADDRMAN, peers.dat and anchors ser/deser seems fine
hebasto:
ACK f8866e8c32, tested on Linux Mint 20.1 (x86_64).
Tree-SHA512: 3898f8a8c51783a46dd0aae03fa10060521f5dd6e79315fe95ba807689e78f202388ffa28c40bf156c6f7b1fc2ce806b155dcbe56027df73d039a55331723796
faf1af58f8 fuzz: Add Temporary debug assert for oss-fuzz issue (MarcoFalke)
Pull request description:
oss-fuzz is acting weird, so add an earlier assert to help troubleshooting
ACKs for top commit:
practicalswift:
cr ACK faf1af58f8
Tree-SHA512: 85830d7d47cf6b4edfe91a07bd5aa8f7110db0bade8df93868cf276ed04d5dd17e671f769e6a0fb5092012b86aa82bb411fb171411f15746981104ce634c88c1
2f5bdcbc31 gui: misc external signer fixes and translation hints (Sjors Provoost)
d672404466 refactor: make ExternalSigner NetworkArg() and m_chain private (Sjors Provoost)
4455145e26 refactor: reduce #ifdef ENABLE_EXTERNAL_SIGNER usage (Sjors Provoost)
5be90c907e build: enable external signer by default (Sjors Provoost)
7d9453041b refactor: clean up external_signer.h includes (Sjors Provoost)
fc0eca31b3 fuzz: fix fuzz binary linking order (Sjors Provoost)
Pull request description:
This follows the introduction of GUI support in https://github.com/bitcoin-core/gui/pull/4
I don't think we should expect GUI users to self compile. This also enables external signer support by default for RPC users.
In addition this PR reduces the number of `#ifdef ENABLE_EXTERNAL_SIGNER`, which also fixes#21919. When compiled with `--disable-external-signer` such wallets can't be created in RPC or GUI, but they can be loaded. Attempting any action that calls HWI will trigger an error.
Side-note: this PR may or may not (currently) break CI for the GUI repository, as explained here: https://github.com/bitcoin-core/gui/pull/4#issuecomment-769859001
ACKs for top commit:
achow101:
ACK 2f5bdcbc31
hebasto:
re-ACK 2f5bdcbc31
Tree-SHA512: 1b71c5a8bea2be077ee9fa33a01130c957a0cf90951d4b7b04d3d0ef826bb77e474c3963abddfef2e2c1ea99d9c72cd2302d1eb9b5fcb7ba0bd2a625f006aa05
We encountered a linking error when attempting to include external_signer_scriptpubkeyman.cpp when configured with --disable-external-signer.
Everywhere else we have LIBBITCOIN_WALLET, it is always before LIBBITCOIN_COMMON. But if you go up to where FUZZ_SUITE_LD_COMMON is first set, you see that we will end up having LIBBITCOIN_COMMON set before LIBBITCOIN_WALLET which means that the linker will have problems linking things common things that the wallet uses. Because the order is correct for the other targets, we only see a linker error for test/fuzz/fuzz.
In this diff, LIBTEST_UTIL and LIBTEST_FUZZ are moved to the top because they include LIBBITCOIN_SERVER and LIBBITCOIN_COMMON. LIBBITCOIN_SERVER always needs to be the first item in the linker order since it has the most dependencies.
The makefiles for making the fuzz and test binaries should be revisited so that the linking order is made consistent with the rest of the code and to avoid other linker order issues that may crop up in the future.
Co-Authored-By: Andrew Chow <achow101-github@achow101.com>
79c02c88b3 Randomize message processing peer order (Pieter Wuille)
Pull request description:
Right now, the message handling loop iterates the list of nodes always in the same order: the order they were connected in (see the `vNodes` vector). For some parts of the net processing logic, this order matters. Transaction requests are assigned explicitly to peers since #19988, but many other parts of processing work on a "first-served-by-loop-first" basis, such as block downloading. If peers can predict this ordering, it may be exploited to cause delays.
As there isn't anything particularly optimal about the current ordering, just make it unpredictable by randomizing.
Reported by Crypt-iQ.
ACKs for top commit:
jnewbery:
ACK 79c02c88b3
Crypt-iQ:
ACK 79c02c88b3
sdaftuar:
utACK 79c02c88b3
achow101:
Code Review ACK 79c02c88b3
jamesob:
crACK 79c02c88b3
jonatack:
ACK 79c02c88b3
vasild:
ACK 79c02c88b3
theStack:
ACK 79c02c88b3
Tree-SHA512: 9a87c4dcad47c2d61b76c4f37f59674876b78f33f45943089bf159902a23e12de7a5feae1a73b17cbc3f2e37c980ecf0f7fd86af9e6fa3a68099537a3c82c106
e4c916a0ea Bugfix: GUI: Use a different shortcut for "1 d&ay" banning, due to conflict with "&Disconnect" (Luke Dashjr)
94e7cdd7e0 GUI: Add keyboard shortcuts for other context menus (Luke Dashjr)
02b5263cd4 GUI: Restore keyboard shortcuts for context menu entries (Luke Dashjr)
Pull request description:
Various keyboard shortcuts were lost in #263; this restores them, and also adds new ones for other context menus.
Note that with a context menu open, simply the shortcut by itself (no Alt) is used.
ACKs for top commit:
jarolrod:
Code Review ACK e4c916a
hebasto:
ACK e4c916a0ea, tested on Linux Mint 20.1 (Qt 5.12.8).
Tree-SHA512: 949461acf7aac592bc48a1c5abad41b167365830e0cedb3aa11b6a87bd347e16126830ea87936f9c9efc4b7df5b09d3833fae784964d6d119ed45703cfba2ffd
The unit test is single threaded, so there's no need to hold the mutex
between Good() and Attempt().
This change avoids recursive locking in the CAddrMan::Attempt function.
Co-authored-by: John Newbery <john@johnnewbery.com>
1b1088d52f test: add combined I2P/onion/localhost eviction protection tests (Jon Atack)
7c2284eda2 test: add tests for inbound eviction protection of I2P peers (Jon Atack)
ce02dd1ef1 p2p: extend inbound eviction protection by network to I2P peers (Jon Atack)
70bbc62711 test: add combined onion/localhost eviction protection coverage (Jon Atack)
045cb40192 p2p: remove unused m_is_onion member from NodeEvictionCandidate struct (Jon Atack)
310fab4928 p2p: remove unused CompareLocalHostTimeConnected() (Jon Atack)
9e889e8a5c p2p: remove unused CompareOnionTimeConnected() (Jon Atack)
787d46bb2a p2p: update ProtectEvictionCandidatesByRatio() doxygen docs (Jon Atack)
1e15acf478 p2p: make ProtectEvictionCandidatesByRatio() fully ratio-based (Jon Atack)
3f8105c4d2 test: remove combined onion/localhost eviction protection tests (Jon Atack)
38a81a8e20 p2p: add CompareNodeNetworkTime() comparator struct (Jon Atack)
4ee7aec47e p2p: add m_network to NodeEvictionCandidate struct (Jon Atack)
7321e6f2fe p2p, refactor: rename vEvictionCandidates to eviction_candidates (Jon Atack)
ec590f1d91 p2p, refactor: improve constness in ProtectEvictionCandidatesByRatio() (Jon Atack)
4a19f501ab test: add ALL_NETWORKS to test utilities (Jon Atack)
519e76bb64 test: speed up and simplify peer_eviction_test (Jon Atack)
1cde800523 p2p, refactor: rm redundant erase_size calculation in SelectNodeToEvict() (Jon Atack)
Pull request description:
Continuing the work in #20197 and #20685, this pull updates and abstracts our inbound eviction protection to make it fully ratio-based and easily extensible to peers connected via high-latency privacy networks that we newly support, like I2P and perhaps others soon, as these peers are disadvantaged by the latency criteria of our eviction logic.
It then adds eviction protection for peers connected over I2P. As described in https://github.com/bitcoin/bitcoin/pull/20685#issuecomment-767486499, we've observed over the past few months that I2P peers have a min ping latency similar to or greater than that of onion peers.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
The goal of this logic is to favorise the diversity of our peer connections.
The individual commit messages describe each change in more detail.
Special thank you to Vasil Dimov for the excellent review feedback and the algorithm improvement that made this change much better than it would have been otherwise. Thanks also to Antoine Riard, whose review feedback nudged this change to protect disadvantaged networks having fewer, rather than more, eviction candidates.
ACKs for top commit:
laanwj:
Code review re-ACK 1b1088d52f
vasild:
ACK 1b1088d52f
Tree-SHA512: 722f790ff11f2969c79e45a5e0e938d94df78df8687e77002f32e3ef5c72a9ac10ebf8c7a9eb7f71882c97ab0e67b2778191effdb747d9ca54d7c23c2ed19a90
