Previously, the validation_chainstatemanager_tests test suite
instantiated its own duplicate ChainstateManager on which tests were
performed.
This wasn't a problem for the specific actions performed in
that suite. However, the existence of this duplicate ChainstateManager
and the fact that many of our validation static functions reach for
g_chainman, ::Chain(state|)Active means we may end up acting on two
different CChainStates should we write more extensive tests in the
future.
This change adds a new ChainTestingSetup which performs all
initialization previously done by TestingSetup except:
1. RPC command registration
2. ChainState initialization
3. Genesis Activation
4. {Ban,Conn,Peer}Man initialization
Means that we will no longer need to initialize a duplicate
ChainstateManger in order to test the initialization codepaths of
CChainState and ChainstateManager.
Lastly, this change has the additional benefit of allowing for
review-only assertions meant to show correctness to work in future work
de-globalizing g_chainman.
In the test chainstatemanager_rebalance_caches, an additional
LoadGenesisBlock call is added as MaybeReblanaceCaches eventually calls
FlushBlockFile, which tries to access vinfoBlockFile[nLastBlockFile],
which is out of bounds when LoadGenesisBlock hasn't been called yet.
-----
Note for the future:
The class con/destructor inheritance structure we have for these
TestingSetup classes is probably not the most suitable abstraction. In
particular, for both TestingSetup and ChainTestingSetup, we need to stop
the scheduler first before anything else. Otherwise classes depending on
the scheduler may be referenced by the scheduler after said classes are
freed. This means that there's no clear parallel between our teardown
code and C++'s destructuring order for class hierarchies.
Future work should strive to coalesce (as much as possible) test and
non-test init codepaths and perhaps structure it in a more fail-proof
way.
These calls are no longer needed after edc316020e8270dafc5e31465d532baebdafd3dd
from #19098 which started instantiating BasicTestingSetup.m_node.chain
Patch from MarcoFalke <falke.marco@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/19425#discussion_r526701954
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
4e28753f60613ecd35cdef87bef5f99c302c3fbd feestimator: encapsulate estimation file logic (Antoine Poinsot)
e8ea6ad9c16997bdc7e22a20eca16e234290b7ff init: don't create a CBlockPolicyEstimator if we don't relay transactions (Antoine Poinsot)
86ff2cf202bfb9d9b50800b8ffe3fead3f77f5fa Remove the remaining fee estimation globals (Antoine Poinsot)
03bfeee957ab7e3b6aece82b9561774648094f54 interface: remove unused estimateSmartFee method from node (Antoine Poinsot)
Pull request description:
If the `blocksonly` mode is turned on after running with transaction
relay enabled for a while, the fee estimation will serve outdated data
to both the internal wallet and to external applications that might be
feerate-sensitive and make use of `estimatesmartfee` (for example a
Lightning Network node).
This has already caused issues (for example https://github.com/bitcoin/bitcoin/issues/16840 (C-lightning), or https://github.com/lightningnetwork/lnd/issues/2562 (LND)) and it seems prudent to fail rather than to give inaccurate values.
This fixes#16840, and closes#16890 which tried to fix the symptoms (RPC) but not the cause as mentioned by sdaftuar :
> If this is a substantial problem, then I would think we should take action to protect our own wallet users as well (rather than hide the results of what our fee estimation would do!).
ACKs for top commit:
MarcoFalke:
re-ACK 4e28753f60 👋
jnewbery:
utACK 4e28753f60613ecd35cdef87bef5f99c302c3fbd
Tree-SHA512: c869cf03b86d8194002970bbc84662dae76874967949b9be0d9a4511a1eabcb1627c38aca3154da9dcece1a4c49ec02bd4f9fcca2ec310986e07904559e63ba8
fa0f4157098ea68169ced44730986d0ed2c3a5aa net: Assume that SetCommonVersion is called at most once per peer (MarcoFalke)
Pull request description:
This restores the check removed in https://github.com/bitcoin/bitcoin/pull/17785#discussion_r503224381
Instead of using `error`, which was used previously, it uses a newly introduced `Assume()`. `error` had several issues:
* It logs unconditionally to the debug log
* It doesn't abort the program when the error is hit in tests
ACKs for top commit:
practicalswift:
cr ACK fa0f4157098ea68169ced44730986d0ed2c3a5aa: patch looks correct
jnewbery:
utACK fa0f4157098ea68169ced44730986d0ed2c3a5aa
Tree-SHA512: cd7424a9485775e8c7093b725f8f52a90d47485185e79bac80f7810e450d0b3fda608d8805e9239094929f7bad2dca3fe772fb78ae606c2399d15405521e136b
This moves the CBlockPolicyEstimator to the NodeContext, which get rids
of two globals and allows us to conditionally create the
CBlockPolicyEstimator (and to remove a circular dep).
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
1e62350ca20898189904a88dfef9ea11ddcd8626 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb854ca827a5a3925394f9e09d29b898 lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca20898189904a88dfef9ea11ddcd8626: patch looks correct!
MarcoFalke:
review ACK 1e62350ca20898189904a88dfef9ea11ddcd8626
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
db058efeb0821cb5022e3b29e0aff3627d7aaf83 sync: use HasReason() in double lock tests (Vasil Dimov)
a21dc469ccf076ca3b07b1adbd8bf667145f1c44 sync: const-qualify the argument of double_lock_detected() (Vasil Dimov)
6d3689fcf6cff397187028344570489db3e6ecf4 sync: print proper lock order location when double lock is detected (Vasil Dimov)
Pull request description:
Before:
```
Assertion failed: detected double lock at src/sync.cpp:153, details in debug log.
```
After:
```
Assertion failed: detected double lock for 'm' in src/test/sync_tests.cpp:40 (in thread ''), details in debug log.
```
ACKs for top commit:
jonasschnelli:
utACK db058efeb0821cb5022e3b29e0aff3627d7aaf83
ajtowns:
ACK db058efeb0821cb5022e3b29e0aff3627d7aaf83
hebasto:
ACK db058efeb0821cb5022e3b29e0aff3627d7aaf83, tested on Linux Mint 20 (x86_64).
Tree-SHA512: 452ddb9a14e44bb174135b39f2219c76eadbb8a6c0e80d64a25f995780d6dbc7b570d9902616db94dbfabaee197b5828ba3475171a68240ac0958fb203a7acdb
17a5f172fa9ec509b1c3f950ee8dfb6f025534d2 fuzz: Make addrman fuzzing harness deterministic (practicalswift)
Pull request description:
Make `CAddrMan` fuzzing harness deterministic.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
Crypt-iQ:
utACK 17a5f172fa9ec509b1c3f950ee8dfb6f025534d2
Tree-SHA512: 725f983745233e9b616782247fa18847e483c074ca4336a5beea8a9009128c3a74b4d50a12662d8ca2177c2e1fc5fc121834df6b459ac0af43c931d77ef7c4d8
f15e780b9e57554c723bc02aa41150ecf3e3a8c9 refactor: Clean up CTxMemPool initializer list (Elle Mouton)
e3310692d0e9720e960b9785274ce1f0b58b4cd7 refactor: Make CTxMemPool::m_check_ratio a const and a constructor argument (Elle Mouton)
9d4b4b2c2c49774523de740d6492ee5b1ee15e74 refactor: Avoid double to int cast for nCheckFrequency (Elle Mouton)
Pull request description:
This PR cleans up the CTxMemPool interface by including the ratio used to determine when a mempool sanity check should run in the constructor of CTxMempool instead of using nCheckFrequency which required a cast from a double to a uint32_t. Since nCheckFrequency (now called m_check_ratio) is set in the constructor and only every read from there after, it can be turned into a const and no longer needs to be guarded by the 'cs' lock.
Since nCheckFrequency/m_check_ratio no longer needs to lock the 'cs' mutux, mutex lock line in the "CTxMempool::check" function can be moved below where the m_check_ratio variable is checked. Since the variable is 0 by default (meaning that "CTxMempool::check" will most likely not run its logic) this saves us from unnecessarily grabbing the lock.
ACKs for top commit:
jnewbery:
utACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9
MarcoFalke:
ACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9 👘
glozow:
utACK f15e780b9e
theStack:
Code Review ACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9
Tree-SHA512: d83f3b5311ca128847b621e5e999c7e1bf0f4e6261d4cc090fb13e229a0f7eecd66ad997f654f50a838baf708d1515740aa3bffc244909a001d01fd5ae398b68
830ddf413934226d0b6ca99165916790cc52ca18 Drop noop gcc version checks (Hennadii Stepanov)
Pull request description:
Since #20413 the minimum required GCC version is 7.
ACKs for top commit:
fanquake:
ACK 830ddf413934226d0b6ca99165916790cc52ca18
Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 sync: detect double lock from the same thread (Vasil Dimov)
4df6567e4cbb4677e8048de2f8008612e1b860b9 sync: make EnterCritical() & push_lock() type safe (Vasil Dimov)
Pull request description:
Double lock of the same (non-recursive) mutex from the same thread would produce an undefined behavior. Detect this from `DEBUG_LOCKORDER` and react similarly to the deadlock detection.
This came up during discussion in another, related PR: https://github.com/bitcoin/bitcoin/pull/19238#discussion_r442394521.
ACKs for top commit:
laanwj:
code review ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
hebasto:
re-ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e
fabecce71909c984504c21fa05f91d5f1b471e8c net: Treat raw message bytes as uint8_t (MarcoFalke)
Pull request description:
Using `uint8_t` from the beginning when messages are `recv`ed has two style benefits:
* The signedness is clear from reading the code, as it does not depend on the architecture
* When passing the bytes on, the need for static signedness casts is dropped, making the code a bit less verbose and more coherent
ACKs for top commit:
laanwj:
Code review ACK fabecce71909c984504c21fa05f91d5f1b471e8c
theStack:
Code Review ACK fabecce71909c984504c21fa05f91d5f1b471e8c
jonatack:
Tested ACK fabecce71909c984504c21fa05f91d5f1b471e8c
Tree-SHA512: e6d9803c78633fde3304faf592afa961ff9462a7912d1da97a24720265274aa10ab4168d71b6ec2756b7448dd42585321afee0e5c889e705be778ce9a330d145
8f7b93047581c67f2133cdb8c7845471de66c30f Drop the leading 0 from the version number (Andrew Chow)
Pull request description:
Removes the leading 0 from the version number. The minor version, which we had been using as the major version, is now the major version. The revision, which we had been using as the minor version, is now the minor version. The revision number is dropped. The build number is promoted to being part of the version number. This also avoids issues where it was accidentally not included in the version number.
The CLIENT_VERSION remains the same format as previous as previously, as the Major version was 0 so it never actually got included in it.
The user agent string formatter is updated to follow this new versioning.
***
Honestly I'm just tired of all of the people asking for "1.0" that maybe this'll shut them up. Skip the whole 1.0 thing and go straight to version 22.0!
Also, this means that the terminology we commonly use lines up with how the variables are named. So major versions are actually bumping the major version number, etc.
ACKs for top commit:
jnewbery:
Code review ACK 8f7b930475
MarcoFalke:
review ACK 8f7b93047581c67f2133cdb8c7845471de66c30f 🎻
Tree-SHA512: b5c3fae14d4c0a9c0ab3b1db7c949ecc0ac3537646306b13d98dd0efc17c489cdd16d43f0a24aaa28e9c4a92ea360500e05480a335b03f9fb308010cdd93a436
fa5ed3b4ca609426b2622cad235e107d33db7b30 net: Use Span in ReceiveMsgBytes (MarcoFalke)
Pull request description:
Pass a data pointer and a size as span in `ReceiveMsgBytes` to get the benefits of a span
ACKs for top commit:
jonatack:
ACK fa5ed3b4ca609426b2622cad235e107d33db7b30 code review, rebased to current master 12a1c3ad1a43634, debug build, unit tests, ran bitcoind/-netinfo/getpeerinfo
theStack:
ACK fa5ed3b4ca609426b2622cad235e107d33db7b30
Tree-SHA512: 89bf111323148d6e6e50185ad20ab39f73ab3a58a27e46319e3a08bcf5dcf9d6aa84faff0fd6afb90cb892ac2f557a237c144560986063bc736a69ace353ab9d
ecc6cf1a3b097b9b5b047282063a0b6779631b83 test: fix creation of std::string objects with \0s (Vasil Dimov)
Pull request description:
A string literal `"abc"` contains a terminating `\0`, so that is 4
bytes. There is no need to write `"abc\0"` unless two terminating
`\0`s are necessary.
`std::string` objects do not internally contain a terminating `\0`, so
`std::string("abc")` creates a string with size 3 and is the same as
`std::string("abc", 3)`.
In `"\01"` the `01` part is interpreted as one number (1) and that is
the same as `"\1"` which is a string like `{1, 0}` whereas `"\0z"` is a
string like `{0, 'z', 0}`. To create a string like `{0, '1', 0}` one
must use `"\0" "1"`.
Adjust the tests accordingly.
ACKs for top commit:
laanwj:
ACK ecc6cf1a3b097b9b5b047282063a0b6779631b83
practicalswift:
ACK ecc6cf1a3b097b9b5b047282063a0b6779631b83 modulo happily green CI
Tree-SHA512: 5eb489e8533a4199a9324b92f7280041552379731ebf7dfee169f70d5458e20e29b36f8bfaee6f201f48ab2b9d1d0fc4bdf8d6e4c58d6102f399cfbea54a219e
Removes the leading 0 from the version number. The minor version, which
we had been using as the major version, is now the major version. The
revision, which we had been using as the minor version, is now the minor
version. The revision number is dropped. The build number is promoted to
being part of the version number. This also avoids issues where it was
accidentally not included in the version number.
The CLIENT_VERSION remains the same format as previous as previously,
the Major version was 0 so that was never a factor in CLIENT_VERSION.
05e82d86b09d914ebce05dbc92a7299cb026847b wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack)
9a670b4f07a6140de809d73cbd7f3e614eb6ea74 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack)
be481b72e24fb6834bd674cd8daee67c6938b42d wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack)
449b730579566459e350703611629e63e54657ed wallet: provide valid values if invalid estimate mode passed (Jon Atack)
6da3afbaee5809ebf6d88efaa3958c505c2d71c7 wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack)
173b5b5fe07d45be5a1e5bc7a5df996f20ab1e85 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack)
7f9835a05abf3e168ad93e7195cbaa4bf61b9b07 wallet: remove fee rates from conf_target helps (Jon Atack)
b7994c01e9a3251536fe6538a22f614774eec82d wallet: add fee_rate unit warnings to bumpfee (Jon Atack)
410e471fa42d3db04e8879c71f8c824dcc151a83 wallet: remove redundant bumpfee fee_rate checks (Jon Atack)
a0d495747320c79b27a83c216dcc526ac8df8f24 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack)
e21212f01b7c41eba13b0479b252053cf482bc1f wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack)
6112cf20d43b0be34fe0edce2ac3e6b27cae1bbe wallet: add CFeeRate ctor doxygen documentation (Jon Atack)
3f7279161347543ce4e997d78ea89a4043491145 wallet: fix bug in RPC send options (Jon Atack)
Pull request description:
This PR builds on #11413 and #20220 to address #19543.
- replace overloading the conf_target and estimate_mode params with `fee_rate` in sat/vB in the sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs
- allow non-actionable conf_target value of `0` and estimate_mode value of `""` to be passed to use `fee_rate` as a positional argument, in addition to as a named argument
- fix a bug in the experimental send RPC described in https://github.com/bitcoin/bitcoin/pull/20220#discussion_r513789526 where args were not being passed correctly into the options values
- update the feerate error message units for these RPCs from BTC/kB to sat/vB
- update the test coverage, help docs, doxygen docs, and some of the RPC examples
- other changes to address the excellent review feedback
See this wallet meeting log for more context: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-06.html#l-309
ACKs for top commit:
achow101:
re-ACK 05e82d8
MarcoFalke:
review ACK 05e82d86b0 did not test and found a few style nits, which can be fixed later 🍯
Xekyo:
tACK 05e82d86b09d914ebce05dbc92a7299cb026847b
Sjors:
utACK 05e82d86b09d914ebce05dbc92a7299cb026847b
Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c
89836a82eec63f93bbe6c3bd6a52be26e71ab54d style: minor improvements as a followup to #19845 (Vasil Dimov)
Pull request description:
Address suggestions:
https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495486760https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495488051https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495730125
ACKs for top commit:
jonatack:
re-ACK 89836a8 change since previous review is replacing std::runtime_error with std::exception, built/ran unit tests with gcc debian 10.2.0-15, then broke a few v3 net_tests involving `BOOST_CHECK_EXCEPTION`, rebuilt, ran `src/test/test_bitcoin -t net_tests -l all` and checked the error reporting.
hebasto:
re-ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d
theStack:
ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d
Tree-SHA512: 36477fdccabe5a8ad91fbabb4655cc363a3a7ca237a98ae6dd4a9fae4a4113762040f864d4ca13a47d081f7d16e5bd487edbfb61ab50a37e4a0424e9bec30b24
79ef8324d4c85ed16a304e98805724b8a59022ac tests: Add fuzzing harness for CConnman (practicalswift)
Pull request description:
Add fuzzing harness for `CConnman`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
review ACK 79ef8324d4c85ed16a304e98805724b8a59022ac
Tree-SHA512: eb9ffae20e939b818f8b9def064544b9a8fcd127ca22d1a54af1afedf1d24143be42419f3a03d684be59a5ff07b29d8bfa34ef2aaf1d9f9f75c4c1aaa90a29a8
fa4234d877ea3193bfd0e18ff68dcb8fb84b47b5 test: Mock IBD in net_processing fuzzers (MarcoFalke)
Pull request description:
Without this the fuzzers fail to detect trivial crasher bugs, such as https://github.com/bitcoin/bitcoin/pull/20317#issuecomment-723047111
ACKs for top commit:
practicalswift:
Tested ACK fa4234d877ea3193bfd0e18ff68dcb8fb84b47b5
Tree-SHA512: ce5da5c0a604b7559805a98ffdde882b44ca4f91b003b493d6e1be230714ce4cccb11dbfc1fc175f9d8fc779551c0a4103ceb4b473552928207d7d78ae329e10
d7901ab8d2fdd2f6e68c4fa48078111bf5f0fa73 fuzz: Assert expected DecodeHexTx behaviour when using legacy decoding (practicalswift)
Pull request description:
Assert expected `DecodeHexTx` behaviour when using legacy decoding.
As suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/20290#issuecomment-720989597.
ACKs for top commit:
MarcoFalke:
review ACK d7901ab8d2fdd2f6e68c4fa48078111bf5f0fa73
Tree-SHA512: 3285680059e6fa73b0fb2c52b775f6319de1ac616f731206662b742764dc888cdfd1ac1f1fcfdfd5418d2006475a852d1c1a56a7035f772f0a6b2a84f5de93bc
5cafe2b25c1d2f6825b3c8103c280020929dd645 fuzz: Add missing ECC_Start to descriptor_parse test (Ivan Metlushko)
Pull request description:
Fixes fuzzing harness.
I also observed that the corpus for this test consists only of `xprv...` keys while we are using regtest parameters. So for proper fuzzing we need either A) to update the corpus and replace `xprv...` with `tprv...` B) switch to main net in the test
ACKs for top commit:
MarcoFalke:
review ACK 5cafe2b25c1d2f6825b3c8103c280020929dd645
practicalswift:
Tested ACK 5cafe2b25c1d2f6825b3c8103c280020929dd645
Tree-SHA512: 7415a98a445ce0f96219637d2362fecfc1191ad104f55d79ca92b0c92cde165e00646be5bf3fda956385e3cb22540eca457e575048493367cdf0e00a27d7cdb8
fa3967efdb07f1d22372f4ee2e602ea1fad04a57 test: Replace ARRAYLEN with C++11 ranged for loop (MarcoFalke)
fafc5290538fde76c3780976f4b2c11dc9f24d19 test: Run AssetTest even if built --with-libs=no (MarcoFalke)
faf58ab139949ca35b33217d010b350c9a59c61d ci: Add --with-libs=no to one ci config (MarcoFalke)
Pull request description:
`script_assets_test` doesn't call libbitcoinconsensus, so it seems confusing to require it
ACKs for top commit:
fanquake:
ACK fa3967efdb07f1d22372f4ee2e602ea1fad04a57 - looks ok to me.
Tree-SHA512: 8744fef64c5d7dc19a0ef4ae9b3df5b3f356253bf000f12723064794c5e50df071824d436059985f1112d94c1b530b65cbeb6b8435114a91b195480620eafc59
c2cf8a18c25bf19ade51fedfa5c352bd7145edb0 fuzz: Check for addrv1 compatibility before using addrv1 serializer on CService (practicalswift)
Pull request description:
Check for addrv1 compatibility before using addrv1 serializer/deserializer on `CService`:
Before this patch:
```
$ src/test/fuzz/service_deserialize
service_deserialize: test/fuzz/deserialize.cpp:85:
void (anonymous namespace)::AssertEqualAfterSerializeDeserialize(const T &, const int) [T = CService]:
Assertion `Deserialize<T>(Serialize(obj, version)) == obj' failed.
```
After this patch:
```
$ src/test/fuzz/service_deserialize
…
```
Related change: #20247
ACKs for top commit:
MarcoFalke:
review ACK c2cf8a18c25bf19ade51fedfa5c352bd7145edb0
Tree-SHA512: dba6ddc60e8ef621011d844281461f1741de08c4af1a2b7156c810af44306cef7ec582de5974752db02ca085cfd23da0296d70b694e59ee262589d829fa0626e
16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751 Avoid test-before-evict evictions of current peers (Suhas Daftuar)
e8b215a086d91a8774210bb6ce8d1560aaaf0789 Refactor test for existing peer connection into own function (Suhas Daftuar)
4fe338ab3ed73b3ffb20eedf95500c56ec2920e1 Call CAddrMan::Good() on block-relay-only peer addresses (Suhas Daftuar)
daf55531260833d597ee599e2d289ea1be0b1d9c Avoid calling CAddrMan::Connected() on block-relay-only peer addresses (Suhas Daftuar)
Pull request description:
This PR does two things:
* Block-relay-only interaction with addrman.
* Calling `CAddrMan::Connected()` on an address that was a block-relay-only peer causes the time we report in `addr` messages containing that peer to be updated; particularly now that we use anchor connections with a our block-relay-only peers, this risks leaking information about those peers. So, stop this.
* Avoiding calling `CAddrMan::Good()` on block-relay-only peer addresses causes the addrman logic around maintaining the new and tried table to be less good, and in particular makes it so that block-relay-only peer addresses are more likely to be evicted from the addrman (for no good reason I can think of). So, mark those addresses as good when we connect.
* Fix test-before-evict bug. There's a bug where if we get a collision in the tried table with an existing address that is one of our current peers, and the connection is long-lived enough, then `SelectTriedCollisions()` might return that existing peer address to us as a test-before-evict connection candidate. However, our logic for new outbound connections would later prevent us from actually making a connection; the result would be that when we get a collision with a long-lived current peer, that peer's address is likely to get evicted from the tried table. Fix this by checking to see if a test-before-evict candidate is a peer we're currently connected to, and if so, mark it as `Good()`.
ACKs for top commit:
sipa:
utACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751
amitiuttarwar:
code review ACK 16d9bfc417
mzumsande:
Code-Review ACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751.
jnewbery:
utACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751
ariard:
Code Review ACK 16d9bfc.
jonatack:
Tested ACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751
Tree-SHA512: 188ccb814e436937cbb91d29d73c316ce83f4b9c22f1cda56747f0949a093e10161ae724e87e4a2d85ac40f85f5f6b4e87e97d350a1ac44f80c57783f4423324
