| Age | Commit message (Collapse) | Author |
|---|
|
fa2ae0ac8d43086430a29c73940ad6b1cd129e96 span: Add Span::empty() and use it in script/descriptor (MarcoFalke)
fa8a99258947a9ee3749fa472180542920cd471c Work around memory-aliasing in descriptor ParsePubkey (MarcoFalke)
Pull request description:
While this is not undefined behaviour, the memory aliasing trick is confusing when reading the code. Having `a.size()==0` and then access `a[0]` works in this particular case, but should probably be avoided to harden the code for the future.
ACKs for top commit:
theStack:
re-ACK https://github.com/bitcoin/bitcoin/pull/19508/commits/fa2ae0ac8d43086430a29c73940ad6b1cd129e96
elichai:
ACK fa2ae0ac8d43086430a29c73940ad6b1cd129e96
jonatack:
ACK fa2ae0ac8d43086430a29c73940ad6b1cd129e96
Tree-SHA512: 0ec7b09eef45504973a195923cdf1aa8522117c8e2f69b453e5ce9aa8a7e327c71138518022c32d05133dc99cb861101ed0f60fa891814ee3e9dab3a6fa61a84
|
|
MaybeDiscourageAndDisconnect()
655b1957470c39bcab64917747c9f467444bd809 [net processing] Continue SendMessages processing if not disconnecting peer (John Newbery)
a49781e56d2bd6a61ec027a09c1db9ee1a4abf2e [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages (John Newbery)
a1d5a428a24afe4f600be29e9d0d3bb4c720e816 [net processing] Fix bad indentation in SendMessages() (John Newbery)
1a1c23f8d40116741f0e26cdf22688fd91c923fc [net processing] Change cs_main TRY_LOCK to LOCK in SendMessages() (John Newbery)
Pull request description:
The motivation for this PR is to reduce the scope of cs_main locking in misbehavior logic. It is the first set of commits from a larger branch to move the misbehavior data out of CNodeState and into a new struct that doesn't take cs_main.
There are some very minor behavior changes in this branch, such as:
- Not checking for discouragement/disconnect in `ProcessMessages()` (and instead relying on the following check in `SendMessages()`)
- Checking for discouragement/disconnect as the first action in `SendMessages()` (and not doing ping message sending first)
- Continuing through `SendMessages()` if `MaybeDiscourageAndDisconnect()` doesn't disconnect the peer (rather than dropping out of `SendMessages()`
ACKs for top commit:
jonatack:
re-ACK 655b195 per `git range-diff 505b4ed f54af5e 655b195`, code/commit messages review, a bit of code history, and debug build.
MarcoFalke:
ACK 655b195747 only some style-nits 🚁
promag:
Code review ACK 655b1957470c39bcab64917747c9f467444bd809.
ariard:
Code Review ACK 655b195
Tree-SHA512: fd6d7bc6bb789f5fb7771fb6a45f61a8faba32af93b766554f562144f9631d15c9cc849a383e71743ef73e610b4ee14853666f6fbf08a3ae35176d48c76c65d3
|
|
9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5700e7a9176d0104d470b83ff9aa3589e8 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)
Pull request description:
Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.
ACKs for top commit:
MarcoFalke:
Approach re-ACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 🌾
jnewbery:
utACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8
Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
|
|
2aac093a3d60e446b85eebdf170ea6bed77bec92 test: Add test coverage for -networkactive option (Hennadii Stepanov)
3c58129b1293742a49aa196cb210ff345a7339e6 net: Log network activity status change unconditionally (Hennadii Stepanov)
62fe6aa87e4cdd8b06207abc1387c68d7bfc04c1 net: Add -networkactive option (Hennadii Stepanov)
Pull request description:
Some Bitcoin Core activity is completely local (offline), e.g., reindexing.
The `setnetworkactive` RPC command is already present. This PR adds the corresponding command-line argument / config option, and allows to start the client with disabled p2p network by providing `-networkactive=0` or `-nonetworkactive`.
This was done while reviewing #16981.
ACKs for top commit:
MarcoFalke:
re-ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92 🏠
LarryRuane:
ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92
Tree-SHA512: 446d791b46d7b556d7694df7b1f88cd4fbc09301fe4eaf036b45cb8166ed806156353cc03788a07b633d5887d5eee30a7c02a2d4307141c8ccc75e0a88145636
|
|
d416ae560e46a4846a3fd5990b7d390d2ef30ec8 walletdb: Introduce WalletDatabase abstract class (Andrew Chow)
2179dbcbcd0b9bef7ad9c907b85294b9a1bccf0f walletdb: Add BerkeleyDatabase::Open dummy function (Andrew Chow)
71d28e7cdca1c8553531bb3a4725d7916363ec5c walletdb: Introduce AddRef and RemoveRef functions (Andrew Chow)
27b27663849932971eb5deadb1f19234b9cd97ea walletdb: Move BerkeleyDatabase::Flush(true) to Close() (Andrew Chow)
Pull request description:
A `WalletDatabase` abstract class is created from `BerkeleyDatabase` and is implemented by `BerkeleyDatabase`. First, to get to the point that this is possible, 4 functions need to be added to `BerkeleyDatabase`: `AddRef`, `RemoveRef`, `Open`, and `Close`.
First the increment and decrement of `mapFileUseCount` is refactored into separate functions `AddRef` and `RemoveRef`.
`Open` is introduced as a dummy function. This will raise an exception so that it always fails.
`Close` is refactored from `Flush`. The `shutdown` argument in `Flush` is removed and instead `Flush(true)` is now the `Close` function.
Split from #18971
Requires #19325
ACKs for top commit:
ryanofsky:
Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8. Only changes since last review were rebasing after base PR #19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids
fjahr:
Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8
meshcollider:
Code review & test run ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8
Tree-SHA512: 98d05ec093d7446c4488e2b0914584222a331e9a2f4d5be6af98e3f6d78fdd8e75526c12f91a8a52d4820c25bce02aa02aabe92d38bee7eb2fce07d0691b7b0d
|
|
|
|
The `setnetworkactive' RPC command is already present.
This new option allows to start the client with disabled p2p network
activity for testing or reindexing.
|
|
0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb Further improve comments around recentRejects (Suhas Daftuar)
0e20cfedb704c1f76bb727e2009867d3d503a03d Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar)
cacd85209e20fc0fd08f86eed23b6ef93484ffcf test: Use wtxid relay generally in functional tests (Fabian Jahr)
8d8099e97ab8af2126f6fbd223fbd82c52f2e85e test: Add tests for wtxid tx relay in segwit test (Fabian Jahr)
9a5392fdf67f1c5c90f52d3cdb3dea4f35d1609f test: Update test framework p2p protocol version to 70016 (Fabian Jahr)
dd78d1d641178b473ab1156b71a837b9e686792b Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar)
4eb515574e1012bc8ea5dafc3042dcdf4c766f26 Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar)
97141ca442daea8fc9c307cf81a02b38dcc28fd8 Delay getdata requests from peers using txid-based relay (Suhas Daftuar)
46d78d47dea345329ba094310eec56ab00a02ddc Add p2p message "wtxidrelay" (Suhas Daftuar)
2d282e0cba9761574b6b43d134ca95f3052d7fd2 ignore non-wtxidrelay compliant invs (Anthony Towns)
ac88e2eb619821ad7ae1d45d4b40be69051d3999 Add support for tx-relay via wtxid (Suhas Daftuar)
8e68fc246d09f1e6c6dfa8c676969d97c2eb4334 Add wtxids to recentRejects instead of txids (Suhas Daftuar)
144c38582042c3b9ec53bb97ba0644fc0114f8fb Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar)
85c78d54af462996a0bca6cf97f91e1aa8778ae8 Add wtxid-index to orphan map (Suhas Daftuar)
08b39955ec7f84e835ab0b1366f0dd28dfd6ce03 Add a wtxid-index to mapRelay (Suhas Daftuar)
60f0acda713e7b9dc188aef54ef93981a93f4e44 Just pass a hash to AddInventoryKnown (Suhas Daftuar)
c7eb6b4f1fe5bd76388a023529977674534334a7 Add wtxid to mempool unbroadcast tracking (Amiti Uttarwar)
2b4b90aa8f0440deacefb5997d7bd1f9f5c591b3 Add a wtxid-index to the mempool (Suhas Daftuar)
Pull request description:
Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool.
We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in #8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this.
As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course).
Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade.
Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome.
To do items:
- [x] Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type)
- [ ] Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes.
ACKs for top commit:
naumenkogs:
utACK 0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb
laanwj:
utACK 0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb
Tree-SHA512: d8eb8f0688cf0cbe9507bf738e143edab1f595551fdfeddc2b6734686ea26e7f156b6bfde38bad8bbbe8bec1857c7223e1687f8f018de7463dde8ecaa8f450df
|
|
fa56eda58e5ec2f2345bbe14c798e83f2abb4728 log: Avoid treating remote misbehvior as local system error (MarcoFalke)
fa492895b572a1196ca8652006b6fc2fa1d16951 refactor: Switch ValidationState mode to C++11 enum class (MarcoFalke)
Pull request description:
When logging failures of `CheckBlockHeader` (high-hash), they are always logged as system error. This is problematic for several reasons:
* Submitting a blockheader that fails `CheckBlockHeader` over RPC will result in a debug log line that starts with `ERROR`. Proper behaviour should be to log not anything and instead only return the failure reason to the RPC user. This pull does not fix this issue entirely, but is a good first step in the right direction.
* A misbehaving peer that sends us an invalid block header that fails `CheckBlockHeader` will result in a debug log line that starts with `ERROR`. Proper behavior should be to log the remote peer misbehavior if logging for that category was enabled. This pull fixes this issue for `CheckBlockHeader` and other functions can be adjusted as well if needed in follow-ups. This should be a good first step in the right direction.
ACKs for top commit:
practicalswift:
re-ACK fa56eda58e5ec2f2345bbe14c798e83f2abb4728
Tree-SHA512: 9793191f5cb57bdff7c93926e94877e8ca2ef89dcebcf9eb155899c733961839ec7c3f9b9f001dc082ada4234fe6e75f6df431301678d6822325840771166d77
|
|
1554b54d47d7e24ce2491f57d24e56d38ceb7649 Static asserts for consistency of fee defaults. (Daniel Kraft)
Pull request description:
This adds `static_assert`'s that ensure that the default values given for fee levels in the wallet (minimum fee and incremental feerate increase) are at least as high as the corresponding levels configured in the core node policy. Since the core policy values are enforced by the network, it makes sense for the wallet to be conservative and above (or at least not below) this.
ACKs for top commit:
laanwj:
code review ACK 1554b54d47d7e24ce2491f57d24e56d38ceb7649, these assumptions seem straightforward
Tree-SHA512: 50e5adf082f467062334377f82a3ee75bcfd436afc65bd0eb33c8d0549d6d90fd1f48c31f60cabe523eb59be9efa8ae0879e9e09cd51ca9c1bd466631ce03cf4
|
|
c8992e89594a54edf283e4916f794475070b5114 test: Fix fuzzer compilation on macOS fixes #19557 (freenancial)
Pull request description:
fixes #19557
Before the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) make
Making all in src
CXX test/fuzz/addition_overflow-addition_overflow.o
In file included from test/fuzz/addition_overflow.cpp:7:
./test/fuzz/util.h:335:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow((uint64_t)fuzzed_file->m_offset, random_bytes.size())) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('unsigned long long' vs. 'unsigned long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
./test/fuzz/util.h:346:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow(fuzzed_file->m_offset, n)) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('long long' vs. 'long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
```
After the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=/usr/local/opt/llvm/bin/clang CXX=/usr/local/opt/llvm/bin/clang++ --disable-asm && make clean && make -j5
...
...
CXXLD test/fuzz/uint256_deserialize
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.
```
ACKs for top commit:
fanquake:
ACK c8992e89594a54edf283e4916f794475070b5114 - tested that compiling works on macOS.
MarcoFalke:
review ACK c8992e89594a54edf283e4916f794475070b5114
Tree-SHA512: 965cdc61b30db0e2209c91b29f0d42de927a9a5b85e1e70f22d1452e0955f876726c7a8c1d1a5f448f12bf24eec3000802071cd4ae28d8605343fd43d174ca84
|
|
c0f09c2c9deaec4cfb35ea587363e6301dd17b88 fuzz: add missing overrides to signature_checker (Jon Atack)
Pull request description:
These functions in `fuzz/signature_checker.cpp` override virtual member functions and should be marked `override` instead of `virtual`, which is for introducing a new virtual function. The overridden virtual functions are in `script/interpreter.h:151/156/161`.
Also, per MarcoFalke suggestion, add missing parentheses in `fuzz/scriptnum_ops.cpp` and remove useless `unsigned int >= 0` conditional in `fuzz/script.cpp`.
These changes fix 5 compile warnings in gcc 10 and 3 in clang 11/12.
ACKs for top commit:
vasild:
ACK c0f09c2
MarcoFalke:
review ACK c0f09c2c9deaec4cfb35ea587363e6301dd17b88
Tree-SHA512: 76ce73ec577c1f23cf8646c31d44dcd6c6303732c47187d041a8921d0d24a50163989a375352ebc221abf2ac337bc0902149be46b6f9eebc071d2f364c407f71
|
|
d0ea9bab2804928c9f40def61fd99064d2d8f9b8 walletdb: Don't remove database transaction logs and instead error (Andrew Chow)
Pull request description:
Instead of removing the database transaction logs and retrying the
wallet loading, just return an error message to the user. Additionally,
speciically for DB_RUNRECOVERY, notify the user that this could be due
to different BDB versions.
Kind of implements the suggestion from https://github.com/bitcoin/bitcoin/pull/18870#discussion_r421647964
ACKs for top commit:
Sjors:
re-utACK d0ea9bab2804928c9f40def61fd99064d2d8f9b8
ryanofsky:
Code review ACK d0ea9bab2804928c9f40def61fd99064d2d8f9b8. Only changes since last review are rebase and expanding error and commit messages.
Tree-SHA512: f6e67dc70f58188742a5c8af7cdc63a2b58779aa0d26ae7f1e75805a239f1a342433860e5a238d6577fae5ab04b9d15e7f11c55b867065dfd13781a6a62e4958
|
|
and also
- add missing parentheses in fuzz/scriptnum_ops.cpp
- remove useless unsigned int conditional in fuzz/script.cpp
These changes fix 5 compile warnings in gcc 10.
|
|
fixes #19557
|
|
blocksonly variables
ec4c6a17e82a6726d95075b43ebd2525c50b37cd scripted-diff: replace MAX_BLOCKS_ONLY_CONNECTIONS with MAX_BLOCK_RELAY_ONLY_CONNECTIONS (glowang)
Pull request description:
We have two different concepts that have similar names: `-blocksonly` and `block-relay-only`, and the similarity of names could lead to confusion. `-blocksonly` disables all local receiving & relaying of transactions (with a few exceptions), while `block-relay-only`means that bitcoind will make 2 additional outbound connections that are only used for block relay.
In net.h and init.cpp, `MAX_BLOCKS_ONLY_CONNECTIONS` is used to represent the maximum number of `block-relay-only` outbound peers, which is 2. But this name sounds ambiguous, and I proposed a better name, `MAX_BLOCK_RELAY_ONLY_CONNECTION`.
ACKs for top commit:
jnewbery:
ACK ec4c6a17e82a6726d95075b43ebd2525c50b37cd
Tree-SHA512: cfa592a7ff936f14d10cfc1e926a51b82bc0feaf104885a41ca8111b906cb3d1ec5536bab143a3cfca70aa49e9575c6995941eb6d3d7f4018d4535712342f155
|
|
5fa067a27d709a8a24b798cbd2459bf5b291c885 Remove unnecessary blockfile SetPos (Tom Harding)
Pull request description:
Nothing could have changed the position since we retrieved it a few statements earlier. This dates from commit 16d5194165c8c83492b95f431a664d98c40ff254.
ACKs for top commit:
LarryRuane:
ACK 5fa067a27d709a8a24b798cbd2459bf5b291c885
Tree-SHA512: 459cc7226e186c231ffb67f0613f550e8eb940f1b8933c3bc4a4e8dd519c8d5d45884e8cfd9347039dab90a093644bbbb31be063baed1c6fc7984b6cb4f17c9f
|
|
4455949d6f0218b40d33d7fe6de6555f8f62192f Make test DoS_mapOrphans deterministic (David Reikher)
Pull request description:
This pull request proposes a solution to make the test `DoS_mapOrphans` in denialofservice_tests.cpp have deterministic coverage.
The `RandomOrphan` function in denialofservice_tests.cpp and the implicitly called function `ecdsa_signature_parse_der_lax` in pubkey.cpp were causing the non-deterministic test coverage.
In the former, if a random orphan was selected the index of which is bigger than the max. orphan index in `mapOrphanTransactions`, the last orphan was returned from `RandomOrphan`. If the random number generated was never large enough, this condition would not be fulfilled and the corresponding branch wouldn't run. The proposed solution is to force one of the 50 dependant orphans to depend on the last orphan in `mapOrphanTransactions` using the newly introduced function `OrphanByIndex` (and passing it a large uint256), forcing this branch to run at least once.
In the latter, if values for ECDSA `R` or `S` (or both) had no leading zeros, some code would not be executed. The solution was to find a constant signature that would be comprised of `R` and `S` values with leading zeros and calling `CPubKey::Verify` at the end of the test with this signature forcing this code to always run at least once at the end even if it hadn't throughout the test.
To test that the coverage is (at least highly likely) deterministic, I ran
`contrib/devtools/test_deterministic_coverage.sh denialofservice_tests/DoS_mapOrphans 1000`
and the result was deterministic coverage across 1000 runs.
Also - removed denialofservice_tests test entry from the list of non-deterministic tests in the coverage script.
ACKs for top commit:
MarcoFalke:
ACK 4455949d6f0218b40d33d7fe6de6555f8f62192f
Tree-SHA512: 987eb1f94b80d5bec4d4944e91ef43b9b8603055750362d4b4665b7f011be27045808aa9f4c6ccf8ae009b61405f9a1b8671d65a843c3328e5b8acce1f1c00a6
|
|
The RandomOrphan function and the function ecdsa_signature_parse_der_lax
in pubkey.cpp were causing non-deterministic test coverage.
Force seed in the beginning of the test to make it deterministic.
The seed is selected carefully so that all branches of the function
ecdsa_signature_parse_der_lax are executed. Prior to this fix, the test
was exhibiting non-deterministic coverage since none of the ECDSA
signatures that were generated during the test had leading zeroes in
either R, S, or both, resulting in some branches of said function not
being executed. The seed ensures that both conditions are hit.
Removed denialofservice_tests test entry from the list of non-deterministic
tests in the coverage script.
|
|
|
|
The functionality is already provided in the BIP32 utility library util/bip32.h
with the exact same name and function signature.
|
|
|
|
|
|
|
|
|
|
Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.
However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.
Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
|
|
Using both txid and wtxid-based relay with peers means that we could sometimes
download the same transaction twice, if announced via two different hashes from
different peers.
Use a heuristic of delaying txid-peer-getdata requests by 2 seconds, if we have
at least one wtxid-based peer.
|
|
When sent to and received from a given peer, enables using wtxid's for
announcing and fetching transactions with that peer.
|
|
|
|
This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
|
|
Previously, we only added txids to recentRejects if we were sure that the
transaction couldn't have had the wrong witness (either because the witness was
malleated or stripped).
In preparation for wtxid-based relay, we can observe that txid == wtxid for
transactions that have no witness, and add the wtxid of rejected transactions,
provided the transaction wasn't a witness-stripped one. This means that we now
add more data to the filter (as prior to this commit, any transaction with a
witness that failed to be accepted was being skipped for inclusion in the
filter) but witness malleation should still not interfere with relay of a valid
segwit transaction, because the txid of a segwit transaction would not be added
to the filter after failing validation.
In the future, having wtxids in the recent rejects filter will allow us to
skip downloading the same wtxid multiple times, once our peers use wtxids for
transaction relay.
|
|
This is in preparation for wtxid-based invs (we need to be able to tell whether
we AlreadyHave() a transaction based on either txid or wtxid).
This also double the size of the bloom filter, which is overkill, but still
uses a manageable amount of memory.
|
|
|
|
|
|
Since it's only used for transactions, there's no need to pass in an inv type.
|
|
|
|
|
|
LoadExternalBlockFile and other FILE* consumers
ad6c34881dc125c973b6b9ba1daa999d3141b1ae tests: Add fuzzing harness for CBlockPolicyEstimator::{Read,Write} (policy/fees.h) (practicalswift)
614e0807a8137d82832aea45e4864b424f71f698 tests: Add fuzzing harness for CBufferedFile::{SetPos,GetPos,GetType,GetVersion} (stream.h) (practicalswift)
7bcc71e5f8cdfd8ba1411c799c0726f503e52343 tests: Add fuzzing harness for LoadExternalBlockFile(...) (validation.h) (practicalswift)
98233760305a36acbd41d76aeebeada1340f6367 tests: Add fuzzing harness for CBufferedFile (streams.h) (practicalswift)
f3aa659be676a4dd0c20fe6c5cb4acd7a5b38b76 tests: Add fuzzing harness for CAutoFile (streams.h) (practicalswift)
e507c0799d759355dd0cfbe83449f0f767a7264e tests: Add serialization/deserialization fuzzing helpers WriteToStream(…)/ReadFromStream(…) (practicalswift)
e48094a506ad031d211b9dfe7639d8b3a2239788 tests: Add FuzzedAutoFileProvider which provides a CAutoFile interface to FuzzedDataProvider (practicalswift)
9dbcd6854ca05a9bd1e9a5e1222dac1758048231 tests: Add FuzzedFileProvider which provides a FILE* interface to FuzzedDataProvider using fopencookie (practicalswift)
Pull request description:
Add fuzzing harnesses for `CAutoFile`, `CBufferedFile`, `LoadExternalBlockFile` and other `FILE*` consumers:
* Add `FuzzedFileProvider` which provides a `FILE*` interface to `FuzzedDataProvider` using `fopencookie`
* Add `FuzzedAutoFileProvider` which provides a `CAutoFile` interface to `FuzzedDataProvider`
* Add serialization/deserialization fuzzing helpers `WriteToStream(…)`/`ReadFromStream(…)`
* Add fuzzing harness for `CAutoFile` (`streams.h`)
* Add fuzzing harness for `CBufferedFile` (`streams.h`)
* Add fuzzing harness for `LoadExternalBlockFile(...)` (`validation.h`)
* Add fuzzing harness for `CBlockPolicyEstimator::Read` and `CBlockPolicyEstimator::Write` (`policy/fees.h`)
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:
Tested ACK ad6c348
Tree-SHA512: a38e142608218496796a527d7e59b74e30279a2815450408b7c27a76ed600cebc6b88491e831665a0639671e2d212453fcdca558500bbadbeb32b267751f8f72
|
|
e80259f1976545e4f1ab6a420644be0c32261773 Additionally treat Tx.nVersion as unsigned in joinpsbts (Matt Corallo)
970de70bdd3542e75b73c79b06f143168c361494 Dump transaction version as an unsigned integer in RPC/TxToUniv (Matt Corallo)
Pull request description:
Consensus-wise we already treat it as an unsigned integer (the
only rules around it are in CSV/locktime handling), but changing
the underlying data type means touching consensus code for a
simple cleanup change, which isn't really worth it.
See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299
ACKs for top commit:
sipa:
ACK e80259f1976545e4f1ab6a420644be0c32261773
practicalswift:
ACK e80259f1976545e4f1ab6a420644be0c32261773
ajtowns:
ACK e80259f1976545e4f1ab6a420644be0c32261773 code review -- checked all other uses of tx.nVersion treat it as unsigned (except for policy.cpp:IsStandard anyway), so looks good.
naumenkogs:
ACK e80259f
Tree-SHA512: 6760a2c77e24e9e1f79a336ca925f9bbca3a827ce02003c71d7f214b82ed3dea13fa7d9f87df9b9445cd58dff8b44a15571d821c876f22f8e5a372a014c9976b
|
|
f58c4b538ebd67fcfea0a4aff5e062fd59fb19f5 [tests] Remove unnecessary cs_mains in denialofservice_tests (Matt Corallo)
Pull request description:
9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
ACKs for top commit:
promag:
ACK f58c4b538ebd67fcfea0a4aff5e062fd59fb19f5.
jonatack:
ACK f58c4b538ebd67fcfe verified the test locks correspond to the locks in net/net_processing, and the debug build is clean/unit tests pass.
Tree-SHA512: de2d9b2a8f08081b2ce31e18585e4677b167a11752b797d790c281575d7dfef3587f8be4fc7f8f16771141b6ff0b0145c7488cf30e79256b0043947c67a6182c
|
|
80968cff68f4d11d98a2a6670846eafbb2803f4f scripted-diff: rename movie folder to animation (Peter Bushnell)
Pull request description:
Rename the movies directory and RES_MOVIES make variable to animation and RES_ANIMATION respectively. Movies is a bit of an unexpected term to be found.
ACKs for top commit:
MarcoFalke:
ACK 80968cff68f4d11d98a2a6670846eafbb2803f4f
hebasto:
ACK 80968cff68f4d11d98a2a6670846eafbb2803f4f, tested on Linux Mint 20 (Qt 5.12.8).
Tree-SHA512: 6bd31ce36e821f6a1bef8a7972086a2387d6258c48fc9df12d3ffdae07d0237036afbc2dec673384b78d9567b91d6e12eafa59fa2305aa79153dfd9b7c3a8655
|
|
784ef8be41c7e5130a6b063b359031ee1ce75aff gui: Show permissions instead of whitelisted (Wladimir J. van der Laan)
Pull request description:
Show detailed permissions instead of legacy "whitelisted" flag in the peer list details.
These are formatted with `&` in between just like services flags. It reuses the "N/A" translation message if there are no special permissions.
This removes the one-but-last use of `legacyWhitelisted`.
Top commit has no ACKs.
Tree-SHA512: 11982da4b9d408c74bc56bb3c540c0eb22506be6353aa4d4d6c64461d140f0587be194e2daad1612fddaa2618025a856b33928ad89041558f418f721f6abd407
|
|
net_processing.cpp
0c8461a88ed66a1f70559fc96646708949b17e4b refactor: replace CConnman pointers by references in net_processing.cpp (Sebastian Falbesoner)
Pull request description:
This is a follow-up to the recently merged PR https://github.com/bitcoin/bitcoin/pull/19053, replacing ~~two more types of~~ one more type of pointer (CConnman) by references to increase the code quality -- pointers should either check for `nullptr` or be replaced by references, and the latter strategy seems to be more reasonable.
Again, to keep the review burden managable, the changes are kept simple,
* only tackling `CConnman*` ~~and `BanMan*`~~ pointers
* only within the net_processing module, i.e. no changes that would need adaption in other modules
* keeping the names of the variables as they are
ACKs for top commit:
jnewbery:
utACK 0c8461a88ed66a1f70559fc96646708949b17e4b
MarcoFalke:
ACK 0c8461a88ed66a1f70559fc96646708949b17e4b 🕧
Tree-SHA512: 79dc05144bcfb5e0bbc62180285aadcc6199f044fa3016c0f54f7b7f45037415260970037bd63b18fafefb8aef448549dae14b780bafb540fa2373f493a17f71
|
|
Show detailed permissions instead of legacy "whitelisted" flag.
These are formatted with `&` in between just like services flags.
It reuses the "N/A" translation message if not.
This removes the one-but-last use of `legacyWhitelisted`.
|
|
bc74a40a56128f81f11151d5966f53b82f19038c net: improve encapsulation of CNetAddr (Vasil Dimov)
Pull request description:
Do not access `CNetAddr::ip` directly from `CService` methods.
This improvement will help later when we change the type of
`CNetAddr::ip` (in the BIP155 implementation).
(chopped off from https://github.com/bitcoin/bitcoin/pull/19031 to ease review)
ACKs for top commit:
dongcarl:
ACK bc74a40a56128f81f11151d5966f53b82f19038c
naumenkogs:
ACK bc74a40
fjahr:
Code review ACK bc74a40
laanwj:
code review ACK bc74a40a56128f81f11151d5966f53b82f19038c
jonatack:
ACK bc74a40a56128f81f11151d5966f53b82f19038c
jnewbery:
ACK bc74a40a5
Tree-SHA512: 29a203905538e8311e3249b78565abe69ce36dc4ec239bec85c726c30e1a7b55b0aaf5c6659b676935008e068cfa53d716f7a598469064108daf130f94329a5d
|
|
0ecff9dd3418e8c18fa423ba53e9cab1df8be553 Improve "detected inconsistent lock order" error message (Hennadii Stepanov)
bbe9cf4fe4ff9a8d1ea557fb763c76100db07679 test: Improve "potential deadlock detected" exception message (Hennadii Stepanov)
35599344c886b62f198e35fd940c1ab15c4a9f90 Fix mistakenly swapped "previous" and "current" lock orders (Hennadii Stepanov)
Pull request description:
In master (8ef15e8a86038225afef2487ca23abc10ca5dffa) the "previous" and "current" lock orders are mistakenly swapped.
This PR:
- fixes printed lock orders
- improves the `sync_tests` unit test
- makes the "detected inconsistent lock order" error message pointing to the lock location rather `tfm::format()` location.
Debugger output example with this PR (with modified code, of course):
```
2020-06-22T15:46:56Z [msghand] POTENTIAL DEADLOCK DETECTED
2020-06-22T15:46:56Z [msghand] Previous lock order was:
2020-06-22T15:46:56Z [msghand] (2) 'cs_main' in net_processing.cpp:2545 (in thread 'msghand')
2020-06-22T15:46:56Z [msghand] (1) 'g_cs_orphans' in net_processing.cpp:1400 (in thread 'msghand')
2020-06-22T15:46:56Z [msghand] Current lock order is:
2020-06-22T15:46:56Z [msghand] (1) 'g_cs_orphans' in net_processing.cpp:2816 (in thread 'msghand')
2020-06-22T15:46:56Z [msghand] (2) 'cs_main' in net_processing.cpp:2816 (in thread 'msghand')
Assertion failed: detected inconsistent lock order for 'cs_main' in net_processing.cpp:2816 (in thread 'msghand'), details in debug log.
Process 131393 stopped
* thread #15, name = 'b-msghand', stop reason = signal SIGABRT
frame #0: 0x00007ffff775c18b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
(lldb) bt
* thread #15, name = 'b-msghand', stop reason = signal SIGABRT
* frame #0: 0x00007ffff775c18b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
frame #1: 0x00007ffff773b859 libc.so.6`__GI_abort at abort.c:79:7
frame #2: 0x0000555555e5b196 bitcoind`(anonymous namespace)::potential_deadlock_detected(mismatch=0x00007fff99ff6f30, s1=size=2, s2=size=2, lock_location=0x00007fff99ff7010) at sync.cpp:134:9
frame #3: 0x0000555555e5a1b1 bitcoind`(anonymous namespace)::push_lock(c=0x0000555556379220, locklocation=0x00007fff99ff7010) at sync.cpp:158:13
frame #4: 0x0000555555e59e8a bitcoind`EnterCritical(pszName="cs_main", pszFile="net_processing.cpp", nLine=2816, cs=0x0000555556379220, fTry=false) at sync.cpp:177:5
frame #5: 0x00005555555b0500 bitcoind`UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::Enter(this=0x00007fff99ff8c20, pszName="cs_main", pszFile="net_processing.cpp", nLine=2816) at sync.h:134:9
frame #6: 0x00005555555b017f bitcoind`UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::UniqueLock(this=0x00007fff99ff8c20, mutexIn=0x0000555556379220, pszName="cs_main", pszFile="net_processing.cpp", nLine=2816, fTry=false) at sync.h:160:13
frame #7: 0x00005555556aa57e bitcoind`ProcessMessage(pfrom=0x00007fff90001180, msg_type=error: summary string parsing error, vRecv=0x00007fff9c005ac0, nTimeReceived=1592840815980751, chainparams=0x00005555564b7110, chainman=0x0000555556380880, mempool=0x0000555556380ae0, connman=0x000055555657aa20, banman=0x00005555565167b0, interruptMsgProc=0x00005555565cae90) at net_processing.cpp:2816:9
```
ACKs for top commit:
laanwj:
ACK 0ecff9dd3418e8c18fa423ba53e9cab1df8be553
vasild:
ACK 0ecff9dd
Tree-SHA512: ff285de8dd3198b5b33c4bfbdadf9b1448189c96143b9696bc4f41c07e784c00851ec169cf3ed45cc325f3617ba6783620803234f57fcce28bf6bc3d6a7234fb
|
|
(server)
fa7592bfa8691eb0289b21da3571709a18391b0f rpc: Update server to use new RPCHelpMan (MarcoFalke)
aaaaad562790cd4dce1568ae193f5393aacacedf rpc: Add option to hide RPCArg (MarcoFalke)
fa9708f94c01cb8bf2971bdf404af38c38fa341b rpc: Assert that passed arg names are equal to hardcoded ones (MarcoFalke)
faaeb2b0b347b40ce456a951eec5e820587e5b02 rpc: Add CRPCCommand constructor which takes RPCHelpMan (MarcoFalke)
fa8ec00061567e56333bb69c5623919d45a9a92d rpc: Check that left section is not multiline (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in server. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
laanwj:
ACK fa7592bfa8691eb0289b21da3571709a18391b0f
ryanofsky:
Code review ACK fa7592bfa8691eb0289b21da3571709a18391b0f. Looks great! Just some hidden arg and Check() and comment cleanups since last review
Tree-SHA512: e64b6a212f4a3aeedeee47557559bde104d5fd40cdc1746b27eb2f3d4c8885d5e6e4dd287595ea11cdbc6a939654fe103cae765fd505875444d851f0abb11308
|
|
9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
|
|
bd315eb5e27d49d47759ae9417328427426cb269 qt: Get rid of cursor in out-of-focus labels (Hennadii Stepanov)
Pull request description:
After clicking on `QLabel` with selectable text the cursor remains forever:
This PR fixes this visual bug.
Earlier attempts to fix this issue:
- #14577
- #14810 (combined with other UX feature)
ACKs for top commit:
promag:
Code review ACK bd315eb5e27d49d47759ae9417328427426cb269.
laanwj:
Tested ACK bd315eb5e27d49d47759ae9417328427426cb269
Tree-SHA512: 6bf89362412e5ce9a4dec6944b62fe44fc31ca49cda7f6e2eb37e847fac9dccb68bca7ac6877b19e42add2333e40d0b4265757ead105ac0a5d28f8ab43b322c3
|
|
fa108d6a757838225179a8df942cfb6d99c98c90 test: update tests for peer discouragement (Jon Atack)
1a9f462caa63fa16d7b4415312d2032a42b3fe0b gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack)
Pull request description:
This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning.
- no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per https://github.com/bitcoin/bitcoin/pull/19464#pullrequestreview-447452052
- update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per https://github.com/bitcoin/bitcoin/pull/19464#issuecomment-658052518
ACKs for top commit:
jnewbery:
ACK fa108d6a757838225179a8df942cfb6d99c98c90
laanwj:
ACK fa108d6a757838225179a8df942cfb6d99c98c90
Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50
|
