314195c8be Remove unnecessary cast in CKey::SignSchnorr (Pieter Wuille)
a1f76cdb22 Remove --disable-openssl-tests for libsecp256k1 configure (Pieter Wuille)
86dbc4d075 Squashed 'src/secp256k1/' changes from be8d9c262f..0559fc6e41 (Pieter Wuille)
Pull request description:
The motivation for this bump is getting rid of a cast in `CKey::SignSchnorr`; the `aux_rand` argument isn't modified by the `secp256k1_schnorrsig_sign` function, but was marked as non-`const` anyway. This is fixed now (bitcoin-core/secp256k1#966), and the cast is removed in this PR.
There are a few other relevant changes:
* (bitcoin-core/secp256k1#956): replaces a runtime-computed table with a precomputed one; this adds arouns 1 MiB to the binary size, but is a step towards significantly simplifying the API. If 1 MiB is too much, it can be reduced by 2 or 4 (or more) for a slight verification performance reduction.
* (bitcoin-core/secp256k1#983): removes (test/bench only) OpenSSL support entirely, removing the need to pass `--disable-openssl-tests` (see #23314).
* (bitcoin-core/secp256k1#810): mild performance increase for 64-bit non-x86 platforms.
* (bitcoin-core/secp256k1#1002): Make aux_rnd32==NULL behave identical to 0x0000..00 (which impacts BIP341/BIP342 signing in Bitcoin Core, making it more strictly BIP340 compliant, though not in a manner that affects security).
ACKs for top commit:
fanquake:
ACK 314195c8be - this includes a nice simplification to the lilbsecp build system (and thus our build system), and fixes issues like #22854. Did a Guix build on x86 (above), as well as a build on arm64 (except for the arm64 host):
Tree-SHA512: 0e048390fc148fbbdf5b98d9cce8c71067564e7d69d97b68347808a9bc45a04f4fc653c392c880d79d5d8b9cf282195520955581ac4f1595f6a948080cf5949d
fa6a548f54 ci: Disable s390x gui tests for now (MarcoFalke)
Pull request description:
ACKs for top commit:
hebasto:
ACK fa6a548f54, tested locally with
Tree-SHA512: 077bcda0b4612dfbf538eb2999ce9f32ec3739c2ed5709f7c4c5d1a39738fbe6e549bd54b237152fd808736c15af1024e67c95dc87b4c3102738543522cb14d7
fa1dc9b36a p2p: Always serialize local timestamp for version msg (MarcoFalke)
Pull request description:
Currently we serialize the local time when connecting to outbound connections and the "adjusted network" time when someone connects to us.
I presume the reason is to avoid a fingerprint in case the local time is misconfigured. However, the fingerprint still exits when:
* The local time goes out-of-sync after timedata is filled up, in which case the adjusted time is *not* adjusted. See comment in `src/timedata.cpp`. (In practise I expect no adjustment to happen after timedata is filled up by one entry more than half its size).
* The local time is off by more than 70 minutes. See `DEFAULT_MAX_TIME_ADJUSTMENT`. While there is a warning in this case, the warning might be missed by the node operator.
* The adjusted time is poisoned by an attacker. This is only a theoretical concern after commit e457513eb1.
Using the adjusted time does help in a the case where the local time is off by a constant less than 70 minutes and the node quickly connects to 5 outbound peers to retrieve the adjusted time.
Still, I think using `GetAdjustedTime` here gives a false sense of security. It will be better for node operators to instead set the correct time.
ACKs for top commit:
naumenkogs:
ACK fa1dc9b36a
laanwj:
Code review ACK fa1dc9b36a
w0xlt:
crACK fa1dc9b
Tree-SHA512: 70a0f4ab3500e6ddcde291620e35273018cefd1d9e94b91ad333e360139ed18862718bb1a9854af2bf79990bf74b05d95492f77d0747c7b9bdd276c020116dcb
8494dcae0e Replace $(AT) with .SILENCE. (Dmitry Goncharov)
Pull request description:
This reduces the amount of syntax noise in the makefiles.
Setting V=1 still enables verbose logging.
The only noticeable difference in behavior is that, unless V=1 is specified, make won't print its own messages like
make: Nothing to be done for 'all', make: 'all' is up to date, or touch <file>, if -t is specified.
ACKs for top commit:
laanwj:
Tested ACK 8494dcae0e
Tree-SHA512: 66b9111229995aa54a9e87f4571648727d89b8529caec651063cdfe5c00a64341371b648701d192b2334df0614617a00c28eaa56c7f08ee9c00127cada0293ab
4d0ac72f3a [fuzz] Add fuzzing harness to compare both implementations of ChaCha20 (stratospher)
65ef93203c [fuzz] Add D. J. Bernstein's implementation of ChaCha20 (stratospher)
Pull request description:
This PR compares Bitcoin Core's implementation of ChaCha20 with D. J. Bernstein's in order to find implementation discrepancies if any.
ACKs for top commit:
laanwj:
Code review ACK 4d0ac72f3a
Tree-SHA512: f826144b4db61b9cbdd7efaaca8fa9cbb899953065bc8a26820a566303b2ab6a17431e7c114635789f0a63fbe3b65cb0bf2ab85baf882803a5ee172af4881544
fab6d6b2d1 Move pindexBestInvalid to ChainstateManager (MarcoFalke)
facd2137ec Move m_failed_blocks to ChainstateManager (MarcoFalke)
fa47b5c100 Move AcceptBlockHeader to ChainstateManager (MarcoFalke)
fa3d62cf7b Move FindForkInGlobalIndex from BlockManager to CChainState (MarcoFalke)
Pull request description:
Move globals or members of the wrong class to the right class.
ACKs for top commit:
naumenkogs:
ACK fab6d6b2d1
Sjors:
ACK fab6d6b2d1
shaavan:
ACK fab6d6b2d1
Tree-SHA512: 926cbdfa22838517497bacb79ed5f521f64117c2aacf96a0176f62831b4713314a32abc0213df5ee067edf63e4a4300f752a26006d36e5aab415bb91209a271f
261dddb924 test: Combine addr generation helper functions (Martin Zumsande)
aeeccd9aa6 test: Fix intermittent issue in p2p_addr_relay.py (Martin Zumsande)
Pull request description:
Fixes#22449 by increasing the mocktime jump (just as in 6168eb06b2), which prevents failures due to rare Poisson timer events, or at least makes them a lot more unlikely.
The second commit combines the addr generation helper functions `setup_addr_msg` and `setup_rand_addr_msg`. It also changes the way `addr.time` is filled to random, because before, if too many addresses (>600) were created in a batch, they would stop being relayed because their timestamp would be too far in the future.
ACKs for top commit:
josibake:
reACK 261dddb924
jnewbery:
utACK 261dddb924
Tree-SHA512: d0eca887de4bc85092730284cf612193d2c12b0a3d624a2bfa5fef4a5890d3b6375c564333c5927425958e4b6ec86b8854b18b2233c7b6f1691d9ddc397948a9
This combines the addr generation helper functions setup_addr_msg
and setup_rand_addr_msg.
It also changes the way addr.time is filled to random, because before,
if too many addresses (>600) were created in a batch, they would stop
being relayed because their timestamp would be too far in the future.
65efbba45d rpcwallet: mention labels are deactivated for ranged descriptors (Antoine Poinsot)
Pull request description:
It was confusing when trying to use it as a blackbox. So mention it so next ones don't have to open the said box :)
See #23749 for context
ACKs for top commit:
Sjors:
utACK 65efbba45d
achow101:
ACK 65efbba45d
Tree-SHA512: d8a3d1f81c16d95855ac2b01e8fd20e83d6dac1721b3da464a9a890e46102992a6882918be87b2a28b929349ee7f1beb1af6c88b22f065fbbb6948275a6d2b8f
62fa61fa4a refactor: remove the wallet folder if the restore fails (w0xlt)
abbb7eccef refactor: Move restorewallet() RPC logic to the wallet section (w0xlt)
4807f73f48 refactor: Implement restorewallet() logic in the wallet section (w0xlt)
Pull request description:
Currently `restorewallet()` logic is written in the RPC layer and it can´t be reused by GUI. So it moves this to the wallet section and then, GUI can access it.
This is necessary to implement the "Restore Wallet" menu item in the GUI (which is already implemented in https://github.com/bitcoin-core/gui/pull/471 ).
This commit also simplifies error handling and adds a new behavior: if the restore fails, the invalid wallet folder is removed.
ACKs for top commit:
achow101:
ACK 62fa61fa4a
shaavan:
crACK 62fa61fa4a
Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39
618f4d2890 test: re-organized array according to order of logs and included 2 more interruption events (seaona)
71115a5e23 test: include two more interruptions points (seaona)
Pull request description:
This PR aims to introduce 2 more interruption points in the process of initialization, in order to make the` feature_init `testcase more complete. These are the following:
-` Checking all blk files are present`
-` init message: Starting network threads`
It is a small improvement for increasing the coverage of potential interruptions, and making sure that the node can restart successfully after these interruptions.
ACKs for top commit:
jamesob:
ACK 618f4d2890, pending CI
jarolrod:
ACK 618f4d2890
Tree-SHA512: 9d709734e298e955709094bb97478ca7f18859874f1ba026f7c9014d87205aea63f6cf2093ebee600eaf82d3245adb11e77fae24a1ae48b69efefd57f3def921
66a20a54a2 build, qt: Drop support for `i686-linux-android` host (Hennadii Stepanov)
Pull request description:
There are no reasons to keep support for `i686-linux-android` host, which is actually broken in master (50c502f54a), and this fact has been unnoticed for months :)
https://github.com/bitcoin/bitcoin/pull/23675#issuecomment-986206434:
> I'm surprised `i686-linux-android` ABI is still supported. I would love to drop it...
https://github.com/bitcoin/bitcoin/pull/23675#issuecomment-991340132
> What is `i686-linux-android`? 32-bit x86 android? is that really a thing?
ACKs for top commit:
prusnak:
utACK 66a20a54a2
Tree-SHA512: 211f794de2fc569f0ade2a4da805b8bfd4ce2ab0930c5d427acc4f5d015fcdc4911f02fc64f6401197f7641aed79944a9594be80c817547be3269cdd721cf79b
The member is unrelated to block storage (BlockManager). It is related
to validation.
Fix the confusion by moving it.
Can be reviewed with
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The helper was moved in commit b026e318c3,
which also mentioned that it could be moved to CChainState. So do that,
as the functionality is not block-storage related.
This also allows to drop one function argument.
faf2614f60 style: Use 4 spaces for indendation, not 5 (MarcoFalke)
fada66fc2c Disallow copies of CChain (MarcoFalke)
Pull request description:
Creating a copy of the chain is not a valid use case in normal operation. Also, it massively degrades performance.
However, it seems to be a mistake that no one looks out for during review:
* https://github.com/bitcoin/bitcoin/pull/22677#discussion_r760400537
Fix this by disallowing it.
ACKs for top commit:
jamesob:
ACK faf2614f60 ([`jamesob/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai`](https://github.com/jamesob/bitcoin/tree/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai))
glozow:
utACK faf2614f60, nice.
prusnak:
utACK faf2614
Tree-SHA512: 27b908c78842e4700e118adb876c09c3d1ec04662310e983309e2cd6fa8ad38c9359ff45f36a804359b9f117e351c4739e651b3e6754c14e6c6fcd7ae5e68342
0559fc6e41 Merge bitcoin-core/secp256k1#988: Make signing table fully static
7dfceceea6 build: Remove #undef hack for ASM in the precomputation programs
bb36fe9be0 ci: Test `make precomp`
d94a37a20c build: Remove CC_FOR_BUILD stuff
ad63bb4c29 build: Prebuild and distribute ecmult_gen table
ac49361ed0 prealloc: Get rid of manual memory management for prealloc contexts
6573c08f65 ecmult_gen: Tidy precomputed file and save space
5eba83f17c ecmult_gen: Precompute tables for all values of ECMULT_GEN_PREC_BITS
5d0dbef018 Merge bitcoin-core/secp256k1#942: Verify that secp256k1_ge_set_gej_zinv does not operate on infinity.
486205aa68 Merge bitcoin-core/secp256k1#920: Test all ecmult functions with many j*2^i combinations
fdb33dd122 refactor: Make PREC_BITS a parameter of ecmult_gen_build_prec_table
5eb519e1f6 ci: reduce TEST_ITERS in memcheck run
e2cf77328a Test ecmult functions for all i*2^j for j=0..255 and odd i=1..255.
61ae37c612 Merge bitcoin-core/secp256k1#1022: build: Windows DLL additions
4f01840b82 Merge bitcoin-core/secp256k1#1027: build: Add a check that Valgrind actually supports a host platform
6ad908aa00 Merge bitcoin-core/secp256k1#1008: bench.c: add `--help` option and ci: move env variables
592661c22f ci: move test environment variable declaration to .cirrus.yml
dcbe84b841 bench: add --help option to bench.
099bad945e Comment and check a parameter for inf in secp256k1_ecmult_const.
6c0be857f8 Verify that secp256k1_ge_set_gej_zinv does not operate on infinity. a->x and a->y should not be used if the infinity flag is set.
4900227451 Merge bitcoin-core/secp256k1#1025: build: replace backtick command substitution with $()
7c7ce872a5 build: Add a check that Valgrind actually supports a host platform
a4875e30a6 refactor: Move default callbacks to util.h
4c94c55bce doc: Remove obsolete hint for valgrind stack size
5106226991 exhaustive_tests: Fix with ecmult_gen table with custom generator
e1a76530db refactor: Make generator a parameter of ecmult_gen_create_prec_table
9ad09f6911 refactor: Rename program that generates static ecmult_gen table
8ae18f1ab3 refactor: Rename file that contains static ecmult_gen table
00d2fa116e ecmult_gen: Make code consistent with comment
3b0c2185ea ecmult_gen: Simplify ecmult_gen context after making table static
2b7c7497ef build: replace backtick command substitution with $()
49f608de47 Merge bitcoin-core/secp256k1#1004: ecmult: fix definition of STRAUSS_SCRATCH_OBJECTS
c0cd7de6d4 build: add -no-undefined to libtool LDFLAGS
fe32a79d35 build: pass win32-dll to LT_INIT
60bf8890df ecmult: fix definition of STRAUSS_SCRATCH_OBJECTS
fecf436d53 Merge bitcoin-core/secp256k1#1019: build: don't append valgrind CPPFLAGS if not installed (macOS)
2e5e4b67df Merge bitcoin-core/secp256k1#1020: doc: remove use of <0xa0> "no break space"
812ff5c747 doc: remove use of 0xa0 "no break space"
214042a170 build: don't append valgrind CPPFLAGS if not installed
e43ba02cfc refactor: Decouple table generation and ecmult_gen context
22dc2c0a0d ecmult_gen: Move table creation to new file and force static prec
793ad9016a Merge bitcoin-core/secp256k1#1010: doc: Minor fixes in safegcd_implementation.md
dc9b6853b7 doc: Minor fixes in safegcd_implementation.md
ea5e8a9c47 Merge bitcoin-core/secp256k1#1012: Fix typos
233297579d Fix typos
7006f1b97f Merge bitcoin-core/secp256k1#1011: ci: Enable -g if we set CFLAGS manually
72de1359e9 ci: Enable -g if we set CFLAGS manually
74c34e727b Merge bitcoin-core/secp256k1#1009: refactor: Use (int)&(int) in boolean context to avoid compiler warning
16d132215c refactor: Use (int)&(int) in boolean context to avoid compiler warning
c74a7b7e51 Merge bitcoin-core/secp256k1#1007: doc: Replace apoelstra's GPG key by jonasnick's GPG key
3b157c48ed doc: Suggest keys.openpgp.org as keyserver in SECURITY.md
73a7472cd0 doc: Replace apoelstra's GPG key by jonasnick's GPG key
515a5dbd02 Merge bitcoin-core/secp256k1#991: Merge all "external" benchmarks into a single bench binary
af6abcb3d0 Make bench support selecting which benchmarks to run
9f56bdf5b9 Merge bench_schnorrsig into bench
3208557ae1 Merge bench_recover into bench
855e18d8a8 Merge bench_ecdh into bench
2a7be678a6 Combine bench_sign and bench_verify into single bench
8fa41201bd Merge bitcoin-core/secp256k1#1002: Make aux_rnd32==NULL behave identical to 0x0000..00.
5324f8942d Make aux_rnd32==NULL behave identical to 0x0000..00.
21c188b3c5 Merge bitcoin-core/secp256k1#943: VERIFY_CHECK precondition for secp256k1_fe_set_int.
3e7b2ea194 Merge bitcoin-core/secp256k1#999: bench_ecmult: improve clarity of output
23e2f66726 bench: don't return 1 in have_flag() if argc = 1
96b1ad2ea9 bench_ecmult: improve clarity of output
20d791edfb Merge bitcoin-core/secp256k1#989: Shared benchmark format for command line and CSV outputs
aa1b889b61 Merge bitcoin-core/secp256k1#996: Fix G.y parity in sage code
044d956305 Fix G.y parity in sage code
b4b130678d create csv file from the benchmark output
26a255beb6 Shared benchmark format for command line and CSV outputs
9526874d14 Merge bitcoin-core/secp256k1#810: Avoid overly-wide multiplications in 5x52 field mul/sqr
920a0e5fa6 Merge bitcoin-core/secp256k1#952: Avoid computing out-of-bounds pointer.
f34b5cae03 Merge bitcoin-core/secp256k1#983: [RFC] Remove OpenSSL testing support
297ce82091 Merge bitcoin-core/secp256k1#966: Make aux_rand32 arg to secp256k1_schnorrsig_sign const
2888640132 VERIFY_CHECK precondition for secp256k1_fe_set_int.
d49011f54c Make _set_fe_int( . , 0 ) set magnitude to 0
bc08599e77 Remove OpenSSL testing support
10f9bd84f4 Merge bitcoin-core/secp256k1#987: Fix unused parameter warnings when building without VERIFY
189f6bcfef Fix unused parameter warnings when building without VERIFY
da0092bccc Merge bitcoin-core/secp256k1#986: tests: remove `secp256k1_fe_verify` from tests.c and modify `_fe_from_storage` to call `_fe_verify`
d43993724d tests: remove `secp256k1_fe_verify` from tests.c and modify `secp256k1_fe_from_storage` to call `secp256k1_fe_verify`
2a3a97c665 Merge bitcoin-core/secp256k1#976: `secp256k1_schnorrsig_sign_internal` should be static
aa5d34a8fe Merge bitcoin-core/secp256k1#783: Make the public API docs more consistent and explicit
72713872a8 Add missing static to secp256k1_schnorrsig_sign_internal
db4667d5e0 Make aux_rand32 arg to secp256k1_schnorrsig_sign const
9a5a87e0f1 Merge bitcoin-core/secp256k1#956: Replace ecmult_context with a generated static array.
20abd52c2e Add tests for pre_g tables.
6815761cf5 Remove ecmult_context.
f20dcbbad1 Correct typo.
16a3cc07e8 Generate ecmult_static_pre_g.h
8de2d86a06 Bump memory limits in advance of making the ecmult context static.
d7ec49a689 Merge bitcoin-core/secp256k1#969: ci: Fixes after Debian release
5d5c74a057 tests: Rewrite code to circument potential bug in clang
3d2f492ceb ci: Install libasan6 (instead of 5) after Debian upgrade
adec5a1638 Add missing null check for ctx and input keys in the public API
f4edfc7581 Improve consistency for NULL arguments in the public interface
9be7b0f083 Avoid computing out-of-bounds pointer.
b53e0cd61f Avoid overly-wide multiplications
git-subtree-dir: src/secp256k1
git-subtree-split: 0559fc6e41b65af6e52c32eb9b1286494412a162
71d33380ed qt: prevent negative values of progressPerHour (HiLivin)
Pull request description:
Added a similar guard to _progressPerHour_ as is placed at _remainingMSecs_.
It prevents the display of negative values like "-0.00%" in some cases.
ACKs for top commit:
hebasto:
ACK 71d33380ed
jarolrod:
ACK 71d3338
shaavan:
reACK 71d33380ed
Tree-SHA512: 5427cdf4441b542196008034355ea00a075adf8b9aeeb383bacdb4e5fbda23d665448a50035aac93cbf401d5d6211d39a2c7c294568d9f5548a5c7579e201c44
fad943821e scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d Use mockable time for peer connection time (MarcoFalke)
fad7ead146 refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e
shaavan:
ACK fad943821e
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
faa6c3d44c net: Drop only invalid entries when reading banlist.json (MarcoFalke)
Pull request description:
All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.
Also suggested in https://github.com/bitcoin/bitcoin/pull/20966#issuecomment-861150204
ACKs for top commit:
laanwj:
Re-ACK faa6c3d44c
Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
27f353d8ef build, android: Fix Android GUI not loading on Qt 5.15 (Igor Cota)
6fc5c772d4 build, qt: use static QAndroidPlatformIntegrationPlugin (Igor Cota)
Pull request description:
PR moved from https://github.com/bitcoin-core/gui/pull/504 as it escaped the confines of `src/qt`.
ACKs for top commit:
hebasto:
re-ACK 27f353d8ef
promag:
utACK 27f353d8ef
Tree-SHA512: 4b6e6b2fb1923b89934f11caa8c05c6f340881689273f0c08916144e623f03fd5b781f1a53af83f6e87dce211fe02a1cb87e5943d13811c791cc8aa458184d9f
2283b9cd1e test: add tests for LoadBlockIndex when using multiple chainstates (James O'Beirne)
0fd599a51a validation: have LoadBlockIndex account for snapshot use (James O'Beirne)
d0c6e61f5d validation: don't modify genesis during snapshot load (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Currently, `BlockManager::LoadBlockIndex` adds all blocks that have downloaded transactions to the active chain state's `setBlockIndexCandidates` set, ignoring the background chain state.
This PR changes ChainstateManager::LoadBlockIndex to update `setBlockIndexCandidates` in the background chain, not just the active chain. In the active chain, the same blocks are added as before. In the background chain, only blocks that have actually been validated, not blocks marked assumed-valid are added so the background chain will continue to download and validate assumed-valid blocks.
ACKs for top commit:
MarcoFalke:
Concept ACK 2283b9cd1e🤽
Sjors:
utACK 2283b9cd1e
Tree-SHA512: 7c9a80802df4722d85d12b78d2e7f628ac5f11cb8be66913d5c3230339bd1220c6723805509d4460826a17d1dc04b0ae172eb7d09ac0ea5dc5e41d77975cbd5e
fa5865a9e3 Reduce size of strencodings decode tables (MarcoFalke)
fad6761cf7 Fix implicit integer sign changes in strencodings (MarcoFalke)
Pull request description:
This removes casts where they are nonsensical (ToUpper/ToLower) and adds them where needed. Then, remove the suppressions.
Also, reduce static memory usage.
ACKs for top commit:
shaavan:
crACK fa5865a9e3
Tree-SHA512: 29850fb616789befad17f71731f322b2238e94ec431cda293629de540f7ab02060a9fc5b7666168ca2592048df5a39a2975177f47771d4d8211d90321052549d
caac999ff0 refactor: remove dependence on AddrManTest (josibake)
f961c477b5 refactor: check Good() in tried_collisions test (josibake)
207f1c825c refactor: make AddrMan::Good return bool (josibake)
Pull request description:
Previously, the `addrman_tried_collisions` test behaved in the following way:
1. add an address to addrman
2. attempt to move the new address to the tried table (using `AddrMan.Good()`)
3. verify that `num_addrs` matched `size()` to check for collisions in the new table
`AddrMan.size()`, however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking `num_addrs - collisions == size()`.
While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.
### solution
To more directly test the tried table, I refactored `AddrMan::Good()` to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling `Good` to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause `Good` to return false. That being said, this is an improvement over the previous testing methodology.
Additionally, having `Good()` return a boolean is useful outside of testing as it allows the caller to handle the case where `Good` is unable to move the entry to the tried table (e.g a063647413/src/rpc/net.cpp (L945)).
### followup
As a follow up to this PR, I plan to look at the following places `Good()` is called and see if it makes sense to handle the case where it is unable to add an entry to tried:
* a063647413/src/rpc/net.cpp (L945)
* a063647413/src/net.cpp (L2067)
* a063647413/src/net_processing.cpp (L2708)
ACKs for top commit:
jnewbery:
utACK caac999ff0
mzumsande:
Code review ACK caac999ff0
Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99
Currently restorewallet() logic is written in the RPC layer
and it can´t be reused by GUI. So it reimplements this in the
wallet and interface sections and then, GUI can access it.
eaf6be0114 [net processing] Do not request transaction relay from feeler connections (John Newbery)
0220b834b1 [test] Add testing for outbound feeler connections (John Newbery)
Pull request description:
Feelers are short-lived connections used to test the viability of peers. The bitcoind node will periodically open feeler connections to addresses in its addrman, wait for a `version` message from the peer, and then close the connection.
Currently, we set `fRelay` to `1` in the `version` message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the `version` message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set `fRelay` to `0` indicating that we do not wish to receive transaction announcements from the peer.
This PR also extends the `addconnection` RPC to allow creating outbound feeler connections from the node to the test framework, and a test to verify that the node sets `fRelay` to `0` in the `version` message to feeler connections.
ACKs for top commit:
naumenkogs:
ACK eaf6be0114
MarcoFalke:
review ACK eaf6be0114🏃
Tree-SHA512: 1c56837dbd0a396fe404a5e39f7459864d15f666664d6b35ad109628b13158e077e417e586bf48946a23bd5cbe63716cb4bf22cdf8781b74dfce6047b87b465a
Rather than try to infer a collision by checking `AddrMan::size`,
check whether or not moving to the tried table was successful by
checking the output from `AddrMan::Good`
If AddrMan::Good is unable to add an entry
to tried (for a number of reasons), return false.
This makes it much easier and cleaner to directly
test for tried collisions. It also allows anyone
calling Good() to handle the case where adding an
address to tried is unsuccessful.
Update docs to doxygen style.
fa19bab90a fuzz: Rework FillNode (MarcoFalke)
fae6e31df7 refactor: Set fSuccessfullyConnected in FillNode (MarcoFalke)
fa3583f856 fuzz: Avoid negative NodeId in ConsumeNode (MarcoFalke)
Pull request description:
Currently `FillNode` is a bit clumsy because it directly modifies memory of `CNode`. This gets in the way of moving that memory to `Peer`. Also, it isn't particularly consistent. See for example https://github.com/bitcoin/bitcoin/pull/21160#discussion_r739206139 .
Fix all issues by sending a `version`/`verack` in `FillNode` and let net_processing figure out the internal details.
ACKs for top commit:
jnewbery:
Strong concept ACK and light code review ACK fa19bab90a
Tree-SHA512: 33261d857c3fa6d5d39d742624009a29178ad5a15eb3fd062da741affa5a4854fd45ed20d59a6bba2fb068cf7b39cad6f95b2910be7cb6afdc27cd7917955b67
