Commit graph

2955 commits

Author SHA1 Message Date
MarcoFalke
7f0f01f0ab
Merge : 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
practicalswift
8c09c0c1d1 fuzz: Avoid time-based "non-determinism" in fuzzing harnesses by using mocked GetTime() 2020-12-01 13:18:34 +00:00
MarcoFalke
dfd0b70088
Merge : 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 : 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 : 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 : refactor: Drop noop gcc version checks
830ddf4139 Drop noop gcc version checks (Hennadii Stepanov)

Pull request description:

  Since  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 : 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
Dmitry Petukhov
89895773b7
Fix length of R check in test/key_tests.cpp:key_signature_tests
The code before the fix only checked the length of R value of the last
signature in the loop, and only for equality (but the length can be
less than 32)

The fixed code checks that length of the R value is less than or equal
to 32 on each iteration of the loop

The BOOST_CHECK(sig.size() <= 70) is merged with sig[3] <= 32 check,
and BOOST_CHECKs are moved outside the loop, for efficiency
2020-11-25 18:07:37 +05:00
Hennadii Stepanov
830ddf4139
Drop noop gcc version checks
Since  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
MarcoFalke
fada14b948
Treat CDataStream bytes as uint8_t
Also, rename CSerializeData to SerializeData
2020-11-23 21:19:50 +01:00
MarcoFalke
fa8bdb048e
refactor: Drop CDataStream constructors in favor of one taking a Span of bytes 2020-11-23 21:19:22 +01:00
MarcoFalke
faa96f841f
Remove unused CDataStream methods 2020-11-23 19:24:54 +01:00
Wladimir J. van der Laan
1b75f2542d
Merge : 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 : 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 : 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 : 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 : 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  and  to address .

  - 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 : refactor: minor whitespace fixups, s/const/constexpr/ and remove template (followup to )
89836a82ee style: minor improvements as a followup to  (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 : 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 : 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 : 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 : 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 : 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 : 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 : 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: 

ACKs for top commit:
  MarcoFalke:
    review ACK c2cf8a18c2

Tree-SHA512: dba6ddc60e8ef621011d844281461f1741de08c4af1a2b7156c810af44306cef7ec582de5974752db02ca085cfd23da0296d70b694e59ee262589d829fa0626e
2020-11-03 09:14:22 +01:00
fanquake
8387f832d6
Merge : 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
practicalswift
c2cf8a18c2 fuzz: Check for addrv1 compatibility before using addrv1 serializer on CService 2020-11-02 21:45:40 +00:00
MarcoFalke
c5ec0367d7
Merge : Only relay Taproot spends if next block has it active
3d0556d410 Increase feature_taproot inactive test coverage (Pieter Wuille)
525cbd425e Only relay Taproot spends if next block has it active (Pieter Wuille)

Pull request description:

  There should be no change to mempool transaction behavior for witness v1 transactions as long as no activation is defined. Until that point, we should treat the consensus rules as under debate, and for soft-fork safety, that means spends should be treated as non-standard.

  It's possible to go further: don't relay them unless the consensus rules are actually active for the next block. This extends non-relay to the period where a deployment is defined, started, locked in, or failed. I see no downsides to this, and the code change is very simple.

ACKs for top commit:
  Sjors:
    utACK 3d0556d410
  MarcoFalke:
    review ACK 3d0556d410 🏓
  jnewbery:
    utACK 3d0556d410

Tree-SHA512: ca625a2981716b4b44e8f3722718fd25fd04e25bf3ca1684924b8974fca49f7c1d438fdd9dcdfbc091a442002e20d441d42c41a0e2096e74a61068da6c60267a
2020-11-02 10:12:06 +01:00
Samuel Dobson
5a6f3c5a01
Merge : Strip any trailing / in -datadir and -blocksdir paths
ad5cef5dfd doc: Update data directory path comments (Hennadii Stepanov)
b19e88230f util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)

Pull request description:

  Wallet names in `listwalletdir` RPC are correct now, even if the `-datadir` path has any number of trailing `/`.

  This PR is an alternative to .

  Fixes .

ACKs for top commit:
  MarcoFalke:
    review ACK ad5cef5dfd 🔙
  promag:
    Code review ACK ad5cef5dfd.
  meshcollider:
    Code review + test run ACK ad5cef5dfd

Tree-SHA512: bccabbd6c18243d48d15b2b27201cc0f5984623dcbc635c8740cf74523f359844c36eadd40391142874fcf452a43880bb6afbf89815ae736e499f9a98143a661
2020-11-02 11:41:38 +13:00
Vasil Dimov
89836a82ee
style: minor improvements as a followup to
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
2020-10-31 17:02:52 +01:00
Vasil Dimov
ecc6cf1a3b
test: fix creation of std::string objects with \0s
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.
2020-10-31 16:02:49 +01:00
Pieter Wuille
525cbd425e Only relay Taproot spends if next block has it active 2020-10-30 15:52:19 -07:00
Wladimir J. van der Laan
924a4ff7eb
Merge : fuzz: Properly initialize PrecomputedTransactionData
fa56d56d4e fuzz: Properly initialize PrecomputedTransactionData (MarcoFalke)

Pull request description:

  Fixes:

  ```
  script_flags: script/interpreter.cpp:1512: bool SignatureHashSchnorr(uint256 &, const ScriptExecutionData &, const T &, uint32_t, uint8_t, SigVersion, const PrecomputedTransactionData &) [T = CTransaction]: Assertion `cache.m_bip341_taproot_ready && cache.m_spent_outputs_ready' failed.
  ==34989== ERROR: libFuzzer: deadly signal
       0x55e90077ff11 in __sanitizer_print_stack_trace (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x20cf11)
       0x55e9006cb068 in fuzzer::PrintStackTrace() (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x158068)
       0x55e9006b01b3 in fuzzer::Fuzzer::CrashCallback() (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x13d1b3)
       0x7f6fb89383bf  (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf)
       0x7f6fb855018a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a)
       0x7f6fb852f858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858)
       0x7f6fb852f728  (/lib/x86_64-linux-gnu/libc.so.6+0x25728)
       0x7f6fb8540f35 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x36f35)
       0x55e9008275bd in bool SignatureHashSchnorr<CTransaction>(uint256&, ScriptExecutionData const&, CTransaction const&, unsigned int, unsigned char, SigVersion, PrecomputedTransactionData const&) /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/interpreter.cpp:1512:5
       0x55e900825a3f in GenericTransactionSignatureChecker<CTransaction>::CheckSchnorrSignature(Span<unsigned char const>, Span<unsigned char const>, SigVersion, ScriptExecutionData const&, ScriptError_t*) const /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/interpreter.cpp:1699:10
       0x55e900832503 in VerifyWitnessProgram(CScriptWitness const&, int, std::vector<unsigned char, std::allocator<unsigned char> > const&, unsigned int, BaseSignatureChecker const&, ScriptError_t*, bool) /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/interpreter.cpp:1900:26
       0x55e90082ecb5 in VerifyScript(CScript const&, CScript const&, CScriptWitness const*, unsigned int, BaseSignatureChecker const&, ScriptError_t*) /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/interpreter.cpp:1977:18
       0x55e9007a9b61 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags.cpp:51:30
       0x55e9007d0b49 in LLVMFuzzerTestOneInput /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz.cpp:36:5
       0x55e9006b1871 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x13e871)
       0x55e9006b0fb5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x13dfb5)
       0x55e9006b38d7 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x1408d7)
       0x55e9006b3c39 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x140c39)
       0x55e9006a290e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x12f90e)
       0x55e9006cb752 in main (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x158752)
       0x7f6fb85310b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
       0x55e9006776ad in _start (/tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/script_flags+0x1046ad)

ACKs for top commit:
  laanwj:
    Code review ACK fa56d56d4e

Tree-SHA512: 5637b0a0d982360df32d8cd39e913395967af02ec746508fc5f2fd649695c58bfaaf18ef76f4ca9da764d34fdd63dfe188317dd41b2ed57534bd4055a05ae870
2020-10-29 11:50:28 +01:00