ad6c34881d tests: Add fuzzing harness for CBlockPolicyEstimator::{Read,Write} (policy/fees.h) (practicalswift)
614e0807a8 tests: Add fuzzing harness for CBufferedFile::{SetPos,GetPos,GetType,GetVersion} (stream.h) (practicalswift)
7bcc71e5f8 tests: Add fuzzing harness for LoadExternalBlockFile(...) (validation.h) (practicalswift)
9823376030 tests: Add fuzzing harness for CBufferedFile (streams.h) (practicalswift)
f3aa659be6 tests: Add fuzzing harness for CAutoFile (streams.h) (practicalswift)
e507c0799d tests: Add serialization/deserialization fuzzing helpers WriteToStream(…)/ReadFromStream(…) (practicalswift)
e48094a506 tests: Add FuzzedAutoFileProvider which provides a CAutoFile interface to FuzzedDataProvider (practicalswift)
9dbcd6854c 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
e80259f197 Additionally treat Tx.nVersion as unsigned in joinpsbts (Matt Corallo)
970de70bdd 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 e80259f197
practicalswift:
ACK e80259f197
ajtowns:
ACK e80259f197 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
f58c4b538e [tests] Remove unnecessary cs_mains in denialofservice_tests (Matt Corallo)
Pull request description:
9fdf05d70c 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 f58c4b538e.
jonatack:
ACK f58c4b538e verified the test locks correspond to the locks in net/net_processing, and the debug build is clean/unit tests pass.
Tree-SHA512: de2d9b2a8f08081b2ce31e18585e4677b167a11752b797d790c281575d7dfef3587f8be4fc7f8f16771141b6ff0b0145c7488cf30e79256b0043947c67a6182c
80968cff68 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 80968cff68
hebasto:
ACK 80968cff68, tested on Linux Mint 20 (Qt 5.12.8).
Tree-SHA512: 6bd31ce36e821f6a1bef8a7972086a2387d6258c48fc9df12d3ffdae07d0237036afbc2dec673384b78d9567b91d6e12eafa59fa2305aa79153dfd9b7c3a8655
784ef8be41 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
0c8461a88e 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 0c8461a88e
MarcoFalke:
ACK 0c8461a88e🕧
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`.
bc74a40a56 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 bc74a40a56
naumenkogs:
ACK bc74a40
fjahr:
Code review ACK bc74a40
laanwj:
code review ACK bc74a40a56
jonatack:
ACK bc74a40a56
jnewbery:
ACK bc74a40a5
Tree-SHA512: 29a203905538e8311e3249b78565abe69ce36dc4ec239bec85c726c30e1a7b55b0aaf5c6659b676935008e068cfa53d716f7a598469064108daf130f94329a5d
fa7592bfa8 rpc: Update server to use new RPCHelpMan (MarcoFalke)
aaaaad5627 rpc: Add option to hide RPCArg (MarcoFalke)
fa9708f94c rpc: Assert that passed arg names are equal to hardcoded ones (MarcoFalke)
faaeb2b0b3 rpc: Add CRPCCommand constructor which takes RPCHelpMan (MarcoFalke)
fa8ec00061 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 fa7592bfa8
ryanofsky:
Code review ACK fa7592bfa8. Looks great! Just some hidden arg and Check() and comment cleanups since last review
Tree-SHA512: e64b6a212f4a3aeedeee47557559bde104d5fd40cdc1746b27eb2f3d4c8885d5e6e4dd287595ea11cdbc6a939654fe103cae765fd505875444d851f0abb11308
9fdf05d70c 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).
bd315eb5e2 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 bd315eb5e2.
laanwj:
Tested ACK bd315eb5e2
Tree-SHA512: 6bf89362412e5ce9a4dec6944b62fe44fc31ca49cda7f6e2eb37e847fac9dccb68bca7ac6877b19e42add2333e40d0b4265757ead105ac0a5d28f8ab43b322c3
fa108d6a75 test: update tests for peer discouragement (Jon Atack)
1a9f462caa 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 fa108d6a75
laanwj:
ACK fa108d6a75
Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50
addf18da95 Call SHA256AutoDetect in benchmark setup (Pieter Wuille)
Pull request description:
It seems `SHA256AutoDetect()` was not being called in benchmarks, making the numbers only reflect the naive implementation. Fix this by calling it in bench_bitcoin's setup.
ACKs for top commit:
fjahr:
tested ACK addf18da95
pstratem:
ACK addf18da95
laanwj:
ACK addf18da95
Tree-SHA512: 3ba4b068145942df1429bf5913e3f685511e6ebeae2c1a3f9b8ac0144f6db1c7df456f88f480a2129f3e1602e3bf6a39530bb96e2c74c03ddb19324cec6799c7
cca7c577d5 tests: Add fuzzing harness for ChaCha20Poly1305AEAD (practicalswift)
2fc4e5916c tests: Add fuzzing harness for ChaCha20 (practicalswift)
e9e8aac029 tests: Add fuzzing harness for CHKDF_HMAC_SHA256_L32 (practicalswift)
ec86ca1aaa tests: Add fuzzing harness for poly1305_auth(...) (practicalswift)
4cee53bba7 tests: Add fuzzing harness for AES256CBCEncrypt/AES256CBCDecrypt (practicalswift)
9352c32325 tests: Add fuzzing harness for AES256Encrypt/AES256Decrypt (practicalswift)
Pull request description:
Add fuzzing harness for `AES{CBC,}256{Encrypt,Decrypt}`, `poly1305_auth`, `CHKDF_HMAC_SHA256_L32`, `ChaCha20` and `ChaCha20Poly1305AEAD`.
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:
laanwj:
ACK cca7c577d5
Tree-SHA512: cff9acefe370c12a3663aa55145371df835479c6ab8f6d81bbf84e0f81a9d6b0d94e45ec545f9dd5e1702744eaa7947a1f4ffed0171f446fc080369161afd740
fa53635381 util: Make Assert work with any value (MarcoFalke)
Pull request description:
Goal is to avoid compile failures
ACKs for top commit:
jonatack:
ACK fa53635381
ryanofsky:
Code review ACK fa53635381. Looks like if argument is an lvalue this effectively does:
Tree-SHA512: a5cf47a8bb2fa1bd8b8895774f33de50ad803165d6f7b520351be1cfcd5612d5d97c51d118461331d30640186c470879e5ad19e3333e09e72685c5e4e4f23079
314b49bd50 gui: Fix regression in GUI console (Hennadii Stepanov)
Pull request description:
The regression was introduced in #19056: if the GUI is running without `-server=1`, the `*txoutset*` call in the console returns "Shutting down".
Fix#19255.
ACKs for top commit:
ryanofsky:
Code review ACK 314b49bd50. Only change since last review is rebase
Tree-SHA512: 8ff85641a5c249858fecb1ab69c7a1b2850af651ff2a94aa41ce352b5b5bc95bc45c41e1767e871b51e647612d09e4d54ede3e20c313488afef5678826c51b62
b82f0ca4d5 walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow)
eac9200814 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow)
Pull request description:
In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`.
The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`.
Part of #18971
Requires #19308 and #19324
ACKs for top commit:
Sjors:
re-utACK b82f0ca4d5
MarcoFalke:
ACK b82f0ca4d5🌘
meshcollider:
LGTM, utACK b82f0ca4d5
Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a
fa9f20b647 log: Properly log txs rejected from mempool (MarcoFalke)
Pull request description:
Currently `CheckTxInputs` rejections from the mempool are the only rejections that log directly and unconditionally to debug.log instead of leaving it to the caller. This has multiple issues:
* A rejected RPC transaction will log a redundant failure reason to debug log. All other failures are merely reported to the RPC user.
* A rejected p2p transaction will log the failure twice. Once with the `MEMPOOLREJ` flag, and once unconditionally.
* A rejected orphan transaction will log no failure.
Fix all issues by simply returning the state to the caller, like it is done for all other rejections.
The patch includes whitespace fixups to highlight relevant parts of the codebase and simplify review.
ACKs for top commit:
naumenkogs:
utACK fa9f20b647
rajarshimaitra:
Concept ACK. Compiled and ran tests. `fa9f20b`
jnewbery:
code review ACK fa9f20b647
Tree-SHA512: 86cc17b2a9239c01c4fc3f254ad48ee1d3883266966b9811030176338b9ac3deaea7ea5babfb8bbf739d7440154e30011fede8f9313175f199d4a062af6494f7
Although we currently don't do this, it should be possible to create a
CConnman or PeerLogicValidation without a Banman instance. Therefore
always check that banman exists before dereferencing the pointer.
Also add comments to the m_banman members of CConnman and
PeerLogicValidation to document that these may be nullptr.
d842e6ac96 doc: Add non-thread-safe note to FeeFilterRounder::round() (Hennadii Stepanov)
Pull request description:
The `FastRandomContext` class is documented as not thread-safe.
This PR adds a relevant note to the `FeeFilterRounder::round()` function declaration.
Close#19254
ACKs for top commit:
MarcoFalke:
self ACK d842e6ac96
practicalswift:
ACK d842e6ac96: explicit is better than implicit
naumenkogs:
ACK d842e6a
Tree-SHA512: 538508f24b9cb29baece6a64108e2c5fc3960768c6475c4f2baf48a4a7bdb96dcef1a74d21a4822e1f8635e1375362986da4e3a20f5644129046a354c4b0a8a0
f32c408f3a Make sure unconfirmed parents are requestable (Pieter Wuille)
c4626bcd21 Drop setInventoryTxToSend based filtering (Pieter Wuille)
43f02ccbff Only respond to requests for recently announced transactions (Pieter Wuille)
b24a17f039 Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille)
a9bc563803 Swap relay pool and mempool lookup (Pieter Wuille)
Pull request description:
This implements the follow-up suggested here: https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627630111 . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15.
This:
* Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627627010).
* Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see https://github.com/bitcoin/bitcoin/pull/18861#discussion_r419695831).
It adds 37 KiB of filter per peer.
This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see #17303), but that is still blocked by dealing properly with NOTFOUNDs (see #18238).
ACKs for top commit:
jnewbery:
reACK f32c408f3
jonatack:
re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review.
ajtowns:
re-ACK f32c408f3a
Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
Instead of removing the database transaction logs and retrying the
wallet loading, just return an error message to the user. Additionally,
specifically for DB_RUNRECOVERY, notify the user that this could be due
to different BDB versions. This error is pretty much only caused by
compiling with a newer version of BDB and then trying to open the wallet
with a version compiled with an older version of BDB.
Qt tests currently are currently using two NodeContext structs at the same
time, one in interfaces::NodeImpl::m_context, and the other in
BasicTestingSetup::m_node, and the tests have hacks transferring state between
them.
Fix this by getting rid of the NodeImpl::m_context struct and making it a
pointer. This way a common BitcoinApplication object can be used for all qt
tests, but they can still have their own testing setups.
Non-test code is changing but non-test behavior is still the same as before.
Motivation for this PR is to be able to remove the
"std::move(test.m_node.connman)" and mempool hacks for swapping individual
NodeContext members in Qt tests, because followup PR #19099 adds yet another
member (wallet_client) that needs to be swapped. After this change, the whole
NodeContext struct can be swapped instead of individual members, so the
workarounds are less fragile and invasive.
