Commit graph

2656 commits

Author SHA1 Message Date
Hennadii Stepanov
c5e3e74f70
sync: Improve CheckLastCritical()
This commit adds actual lock stack logging if check fails.
2020-12-10 20:46:29 +02:00
John Newbery
68334b3944 [net processing] Add m_ignores_incoming_txs to PeerManager and use internally 2020-12-09 18:13:37 +00:00
Carl Dong
81137c60fe test: Add new ChainTestingSetup and use it
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.
2020-12-08 15:00:25 -05:00
Russell Yanofsky
5baa88fd38 test: Remove no longer needed MakeChain calls
These calls are no longer needed after edc316020e
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>
2020-12-07 20:46:03 -05:00
Russell Yanofsky
6965f1352d refactor: Replace uses ChainActive() in interfaces/chain.cpp
Suggested https://github.com/bitcoin/bitcoin/pull/19425#discussion_r456236407
2020-12-07 09:09:53 -04:00
Russell Yanofsky
3fbbb9a640 refactor: Get rid of more redundant chain methods
This just drops three interfaces::Chain methods replacing them with other calls.

Motivation for removing these chain methods:

- Need to get rid of findFirstBlockWithTimeAndHeight for #10102, which doesn't
  support overloaded methods
- Followup from
  https://github.com/bitcoin/bitcoin/pull/16426#discussion_r412487403
- phantomcircuit comments about findNextBlock test
  http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214

Behavior is not changing in any way here. A TODO comment in
ScanForWalletTransactions was removed, but just because it was invalid (see
https://github.com/bitcoin/bitcoin/pull/19195#discussion_r448020762), not
because it was implemented.
2020-12-07 09:09:53 -04:00
MarcoFalke
03b1db6114
Merge #18766: Disable fee estimation in blocksonly mode (by removing the fee estimates global)
4e28753f60 feestimator: encapsulate estimation file logic (Antoine Poinsot)
e8ea6ad9c1 init: don't create a CBlockPolicyEstimator if we don't relay transactions (Antoine Poinsot)
86ff2cf202 Remove the remaining fee estimation globals (Antoine Poinsot)
03bfeee957 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 4e28753f60

Tree-SHA512: c869cf03b86d8194002970bbc84662dae76874967949b9be0d9a4511a1eabcb1627c38aca3154da9dcece1a4c49ec02bd4f9fcca2ec310986e07904559e63ba8
2020-12-07 12:59:48 +01:00
MarcoFalke
00f4dcd552
Merge #20138: net: Assume that SetCommonVersion is called at most once per peer
fa0f415709 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 fa0f415709: patch looks correct
  jnewbery:
    utACK fa0f415709

Tree-SHA512: cd7424a9485775e8c7093b725f8f52a90d47485185e79bac80f7810e450d0b3fda608d8805e9239094929f7bad2dca3fe772fb78ae606c2399d15405521e136b
2020-12-07 12:48:55 +01:00
practicalswift
12dcdaaa54 Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const 2020-12-06 00:22:40 +00:00
MarcoFalke
fa0f415709
net: Assume that SetCommonVersion is called at most once per peer 2020-12-04 11:19:15 +01:00
Antoine Poinsot
86ff2cf202
Remove the remaining fee estimation globals
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>
2020-12-03 12:56:37 +01:00
fanquake
0a13d15c14
Merge #20530: lint, refactor: Update cppcheck linter to c++17 and improve explicit usage
1e62350ca2 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb 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 1e62350ca2: patch looks correct!
  MarcoFalke:
    review ACK 1e62350ca2

Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
2020-12-02 20:52:19 +08:00
MarcoFalke
7f0f01f0ab
Merge #20507: sync: print proper lock order location when double lock is detected
db058efeb0 sync: use HasReason() in double lock tests (Vasil Dimov)
a21dc469cc sync: const-qualify the argument of double_lock_detected() (Vasil Dimov)
6d3689fcf6 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 db058efeb0
  ajtowns:
    ACK db058efeb0
  hebasto:
    ACK db058efeb0, tested on Linux Mint 20 (x86_64).

Tree-SHA512: 452ddb9a14e44bb174135b39f2219c76eadbb8a6c0e80d64a25f995780d6dbc7b570d9902616db94dbfabaee197b5828ba3475171a68240ac0958fb203a7acdb
2020-12-02 09:44:55 +01:00
Fabian Jahr
1e62350ca2
refactor: Improve use of explicit keyword 2020-12-01 18:36:39 +01:00
MarcoFalke
dfd0b70088
Merge #20425: fuzz: Make CAddrMan fuzzing harness deterministic
17a5f172fa 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 17a5f172fa

Tree-SHA512: 725f983745233e9b616782247fa18847e483c074ca4336a5beea8a9009128c3a74b4d50a12662d8ca2177c2e1fc5fc121834df6b459ac0af43c931d77ef7c4d8
2020-12-01 14:04:10 +01:00
MarcoFalke
cd720337fe
Merge #20222: refactor: CTxMempool constructor clean up
f15e780b9e refactor: Clean up CTxMemPool initializer list (Elle Mouton)
e3310692d0 refactor: Make CTxMemPool::m_check_ratio a const and a constructor argument (Elle Mouton)
9d4b4b2c2c 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 f15e780b9e
  MarcoFalke:
    ACK f15e780b9e 👘
  glozow:
    utACK f15e780b9e
  theStack:
    Code Review ACK f15e780b9e

Tree-SHA512: d83f3b5311ca128847b621e5e999c7e1bf0f4e6261d4cc090fb13e229a0f7eecd66ad997f654f50a838baf708d1515740aa3bffc244909a001d01fd5ae398b68
2020-12-01 10:02:56 +01:00
MarcoFalke
81d5af42f4
Merge #20499: Remove obsolete NODISCARD ifdef forest. Use [[nodiscard]] (C++17).
79bff8e48a Remove NODISCARD (practicalswift)
4848e71107 scripted-diff: Use [[nodiscard]] (C++17) instead of NODISCARD (practicalswift)

Pull request description:

  Remove obsolete `NODISCARD` `ifdef` forest. Use `[[nodiscard]]` (C++17).

ACKs for top commit:
  theStack:
    ACK 79bff8e48a
  fanquake:
    ACK 79bff8e48a

Tree-SHA512: 56dbb8e50ed97ecfbce28cdc688a01146108acae49a943e338a8f983f7168914710d36e38632f6a7c200ba6c6ac35b2519e97d6c985e8e7eb23223f13bf985d6
2020-11-30 15:42:36 +01:00
fanquake
817aeca57a
Merge #20491: refactor: Drop noop gcc version checks
830ddf4139 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 830ddf4139

Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
2020-11-30 16:10:41 +08:00
Vasil Dimov
db058efeb0
sync: use HasReason() in double lock tests
`HasReason()` is shorter than a lambda function.
2020-11-26 14:42:06 +01:00
practicalswift
4848e71107 scripted-diff: Use [[nodiscard]] (C++17) instead of NODISCARD
-BEGIN VERIFY SCRIPT-
sed -i "s/NODISCARD/[[nodiscard]]/g" $(git grep -l "NODISCARD" ":(exclude)src/bench/nanobench.h" ":(exclude)src/attributes.h")
-END VERIFY SCRIPT-
2020-11-26 09:05:59 +00:00
Wladimir J. van der Laan
50091592dd
Merge #19337: sync: detect double lock from the same thread
95975dd08d sync: detect double lock from the same thread (Vasil Dimov)
4df6567e4c 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 95975dd08d
  hebasto:
    re-ACK 95975dd08d

Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e
2020-11-25 17:02:20 +01:00
Hennadii Stepanov
830ddf4139
Drop noop gcc version checks
Since #20413 the minimum required GCC version is 7.

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
2020-11-25 14:38:33 +02:00
practicalswift
05c1095388 test: Add testing of ParseInt/ParseUInt edge cases with leading +/-/0:s 2020-11-24 08:36:48 +00:00
Wladimir J. van der Laan
1b75f2542d
Merge #20432: net: Treat raw message bytes as uint8_t
fabecce719 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 fabecce719
  theStack:
    Code Review ACK fabecce719
  jonatack:
    Tested ACK fabecce719

Tree-SHA512: e6d9803c78633fde3304faf592afa961ff9462a7912d1da97a24720265274aa10ab4168d71b6ec2756b7448dd42585321afee0e5c889e705be778ce9a330d145
2020-11-23 10:26:25 +01:00
MarcoFalke
d4159984c3
Merge #20223: build: Drop the leading 0 from the version number
8f7b930475 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 8f7b930475 🎻

Tree-SHA512: b5c3fae14d4c0a9c0ab3b1db7c949ecc0ac3537646306b13d98dd0efc17c489cdd16d43f0a24aaa28e9c4a92ea360500e05480a335b03f9fb308010cdd93a436
2020-11-20 15:42:07 +01:00
MarcoFalke
fabecce719
net: Treat raw message bytes as uint8_t 2020-11-20 15:11:21 +01:00
Wladimir J. van der Laan
fdd068507d
Merge #20056: net: Use Span in ReceiveMsgBytes
fa5ed3b4ca 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 fa5ed3b4ca code review, rebased to current master 12a1c3ad1a, debug build, unit tests, ran bitcoind/-netinfo/getpeerinfo
  theStack:
    ACK fa5ed3b4ca

Tree-SHA512: 89bf111323148d6e6e50185ad20ab39f73ab3a58a27e46319e3a08bcf5dcf9d6aa84faff0fd6afb90cb892ac2f557a237c144560986063bc736a69ace353ab9d
2020-11-20 06:10:58 +01:00
Wladimir J. van der Laan
2878167c18
Merge #20000: test: fix creation of "std::string"s with \0s
ecc6cf1a3b 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 ecc6cf1a3b
  practicalswift:
    ACK ecc6cf1a3b modulo happily green CI

Tree-SHA512: 5eb489e8533a4199a9324b92f7280041552379731ebf7dfee169f70d5458e20e29b36f8bfaee6f201f48ab2b9d1d0fc4bdf8d6e4c58d6102f399cfbea54a219e
2020-11-20 04:39:37 +01:00
practicalswift
17a5f172fa fuzz: Make addrman fuzzing harness deterministic 2020-11-19 17:21:55 +00:00
Andrew Chow
8f7b930475 Drop the leading 0 from the version number
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.
2020-11-18 12:00:57 -05:00
MarcoFalke
80e32e120e
Merge #20305: wallet: introduce fee_rate sat/vB param/option
05e82d86b0 wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack)
9a670b4f07 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack)
be481b72e2 wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack)
449b730579 wallet: provide valid values if invalid estimate mode passed (Jon Atack)
6da3afbaee wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack)
173b5b5fe0 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack)
7f9835a05a wallet: remove fee rates from conf_target helps (Jon Atack)
b7994c01e9 wallet: add fee_rate unit warnings to bumpfee (Jon Atack)
410e471fa4 wallet: remove redundant bumpfee fee_rate checks (Jon Atack)
a0d4957473 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack)
e21212f01b wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack)
6112cf20d4 wallet: add CFeeRate ctor doxygen documentation (Jon Atack)
3f72791613 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 05e82d86b0
  Sjors:
    utACK 05e82d86b0

Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c
2020-11-17 13:49:12 +01:00
MarcoFalke
2fa085a5d7
Merge #20033: refactor: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845)
89836a82ee style: minor improvements as a followup to #19845 (Vasil Dimov)

Pull request description:

  Address suggestions:
  https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495486760
  https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495488051
  https://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 89836a82ee
  theStack:
    ACK 89836a82ee

Tree-SHA512: 36477fdccabe5a8ad91fbabb4655cc363a3a7ca237a98ae6dd4a9fae4a4113762040f864d4ca13a47d081f7d16e5bd487edbfb61ab50a37e4a0424e9bec30b24
2020-11-16 07:57:34 +01:00
practicalswift
d04a17a790 fuzz: Use ConsumeRandomLengthBitVector(...) in src/test/fuzz/connman and src/test/fuzz/net 2020-11-12 15:33:43 +00:00
practicalswift
e6bb9fde85 tests: Add fuzzing harness for CAddrMan 2020-11-12 14:23:17 +00:00
Jon Atack
173b5b5fe0
wallet: update fee rate units, use sat/vB for fee_rate error messages
and BTC/kvB for feeRate error messages.
2020-11-12 11:43:03 +01:00
MarcoFalke
8a486158cb
Merge #20188: tests: Add fuzzing harness for CConnman
79ef8324d4 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 79ef8324d4

Tree-SHA512: eb9ffae20e939b818f8b9def064544b9a8fcd127ca22d1a54af1afedf1d24143be42419f3a03d684be59a5ff07b29d8bfa34ef2aaf1d9f9f75c4c1aaa90a29a8
2020-11-12 10:06:42 +01:00
practicalswift
3c77b8009d fuzz: Improve coverage for CPartialMerkleTree fuzzing harness 2020-11-11 22:31:40 +00:00
MarcoFalke
fa8dd34e91
Merge #20332: test: Mock IBD in net_processing fuzzers
fa4234d877 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 fa4234d877

Tree-SHA512: ce5da5c0a604b7559805a98ffdde882b44ca4f91b003b493d6e1be230714ce4cccb11dbfc1fc175f9d8fc779551c0a4103ceb4b473552928207d7d78ae329e10
2020-11-10 19:51:11 +01:00
practicalswift
79b8f8d574 fuzz: Assert roundtrip equality for both addrv1 and addrv2 versions of CService 2020-11-09 15:29:15 +00:00
practicalswift
0e3a78a8ab fuzz: Check for addrv1 compatibility before using addrv1 serializer/deserializer on CSubNet 2020-11-09 15:27:41 +00:00
MarcoFalke
fa4234d877
test: Mock IBD in net_processing fuzzers 2020-11-07 07:50:59 +01:00
MarcoFalke
f33e332541
Merge #20303: fuzz: Assert expected DecodeHexTx behaviour when using legacy decoding
d7901ab8d2 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 d7901ab8d2

Tree-SHA512: 3285680059e6fa73b0fb2c52b775f6319de1ac616f731206662b742764dc888cdfd1ac1f1fcfdfd5418d2006475a852d1c1a56a7035f772f0a6b2a84f5de93bc
2020-11-05 07:57:28 +01:00
practicalswift
d7901ab8d2 fuzz: Assert expected DecodeHexTx behaviour when using legacy decoding 2020-11-04 23:11:50 +00:00
MarcoFalke
6760088015
Merge #20300: fuzz: Add missing ECC_Start to descriptor_parse test
5cafe2b25c 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 5cafe2b25c
  practicalswift:
    Tested ACK 5cafe2b25c

Tree-SHA512: 7415a98a445ce0f96219637d2362fecfc1191ad104f55d79ca92b0c92cde165e00646be5bf3fda956385e3cb22540eca457e575048493367cdf0e00a27d7cdb8
2020-11-04 20:38:18 +01:00
Ivan Metlushko
5cafe2b25c fuzz: Add missing ECC_Start to descriptor_parse test 2020-11-04 22:55:03 +07:00
MarcoFalke
88776c2926
Merge #20245: test: Run script_assets_test even if built --with-libs=no
fa3967efdb test: Replace ARRAYLEN with C++11 ranged for loop (MarcoFalke)
fafc529053 test: Run AssetTest even if built --with-libs=no (MarcoFalke)
faf58ab139 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 fa3967efdb - looks ok to me.

Tree-SHA512: 8744fef64c5d7dc19a0ef4ae9b3df5b3f356253bf000f12723064794c5e50df071824d436059985f1112d94c1b530b65cbeb6b8435114a91b195480620eafc59
2020-11-04 08:51:14 +01:00
MarcoFalke
218fe60d91
Merge #20290: fuzz: Fix DecodeHexTx fuzzing harness issue
28f8cb13d4 fuzz: Fix DecodeHexTx fuzzing harness issue (practicalswift)

Pull request description:

  Fix `DecodeHexTx` fuzzing harness issue.

  Before this patch:

  ```
  $ src/test/fuzz/decode_tx
  decode_tx: test/fuzz/decode_tx.cpp:29:
      void test_one_input(const std::vector<uint8_t> &):
      Assertion `result_try_witness_and_maybe_no_witness' failed.
  …
  ```

  After this patch:

  ```
  $ src/test/fuzz/decode_tx
  …
  ```

ACKs for top commit:
  MarcoFalke:
    review ACK 28f8cb13d4

Tree-SHA512: 2ed11b2f00a4c6fa3e8eea76a2a37d89a4b8d52815264676fe3de0a26ad7906cfafda9b843ceede2fd428815472e01fd1f87afb851282a8c7839bd4c87dc382b
2020-11-03 09:48:15 +01:00
MarcoFalke
5174b534da
Merge #20289: fuzz: Check for addrv1 compatibility before using addrv1 serializer/deserializer on CService
c2cf8a18c2 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 c2cf8a18c2

Tree-SHA512: dba6ddc60e8ef621011d844281461f1741de08c4af1a2b7156c810af44306cef7ec582de5974752db02ca085cfd23da0296d70b694e59ee262589d829fa0626e
2020-11-03 09:14:22 +01:00
fanquake
8387f832d6
Merge #20187: Addrman: test-before-evict bugfix and improvements for block-relay-only peers
16d9bfc417 Avoid test-before-evict evictions of current peers (Suhas Daftuar)
e8b215a086 Refactor test for existing peer connection into own function (Suhas Daftuar)
4fe338ab3e Call CAddrMan::Good() on block-relay-only peer addresses (Suhas Daftuar)
daf5553126 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 16d9bfc417
  amitiuttarwar:
    code review ACK 16d9bfc417
  mzumsande:
    Code-Review ACK 16d9bfc417.
  jnewbery:
    utACK 16d9bfc417
  ariard:
    Code Review ACK 16d9bfc.
  jonatack:
    Tested ACK 16d9bfc417

Tree-SHA512: 188ccb814e436937cbb91d29d73c316ce83f4b9c22f1cda56747f0949a093e10161ae724e87e4a2d85ac40f85f5f6b4e87e97d350a1ac44f80c57783f4423324
2020-11-03 09:38:35 +08:00
practicalswift
28f8cb13d4 fuzz: Fix DecodeHexTx fuzzing harness issue 2020-11-02 22:21:03 +00:00