fa8919889f bench: Remove redundant copy constructor in mempool_stress (MarcoFalke)
29f8434368 refactor: Remove redundant PSBT copy constructor (Hennadii Stepanov)
Pull request description:
I fail to see why people add these copy constructors manually without explanation, when the compiler can generate them at least as good automatically with less code.
ACKs for top commit:
promag:
ACK fa8919889f.
hebasto:
ACK fa8919889f, nit s/constructor/operator/ in commit fa8919889f message, as @promag [mentioned](https://github.com/bitcoin/bitcoin/pull/17349#discussion_r341776389) above.
jonatack:
ACK fa8919889f
Tree-SHA512: ce024fdb894328f41037420b881169b8b1b48c87fbae5f432edf371a35c82e77e21468ef97cda6f54d34f1cf9bb010235d62904bb0669793457ed1c3b2a89723
fa0a731d00 test: Add RegTestingSetup to setup_common (MarcoFalke)
fa54b3e248 test: move-only ComputeFilter to src/test/lib/blockfilter (MarcoFalke)
Pull request description:
The default chain for `TestingSetup` is the main chain. However, any test that wants to mine blocks on demand needs to switch to regtest. This is done manually and in-line right now.
Fix that by creating an explicit `RegTestingSetup` and use it where appropriate.
Also, add a move-only commit to move `ComputeFilter` into the newly created unit test library.
Both commits are part of #15845, but split up because they are useful on their own.
ACKs for top commit:
practicalswift:
ACK fa0a731d00 -- diff looks correct
Tree-SHA512: 02b9765580b355ed8d1be555f8ae11fa6e3d575f5cb177bbdda0319378837e29de5555c126c477dc8a1e8a5be47335afdcff152cf2dea2fbdd1a988ddde3689b
ac831339cb doc: Fix some misspellings (randymcmillan)
Pull request description:
Here is a more thorough lint-spelling update.
This PR takes care of easy to fix spelling errors to clean up the linting stages.
There are misspellings coded into the functional tests.
That is a whole separate job within itself.
ACKs for top commit:
practicalswift:
ACK ac831339cb -- diff looks correct
Tree-SHA512: d8fad83fed083715655f148263ddeffc6752c8007d568fcf3dc2c418ccd5db70089ce3ccfd3994fcbd78043171402eb9cca5bdd5125287e22c42ea305aaa6e9d
f44abe4bed refactor: Remove addrdb.h dependency from node.h (Hennadii Stepanov)
Pull request description:
`node.h` includes `addrdb.h` just for the sake of `banmap_t` type.
This PR makes dependencies simpler and explicit.
~Also needless `typedef` has been removed from `enum BanReason`.~
ACKs for top commit:
laanwj:
ACK f44abe4bed
practicalswift:
ACK f44abe4bed
Tree-SHA512: 33a1be20e5c629daf4a61ebbf93ea6494b9256887cebd4974de4782f6d324404b6cc84909533d9502b2cc19902083f1f9307d4fb7231e67db5b412b842d13072
3ed8e3d079 doc: Remove explicit network name references (Fabian Jahr)
d6e493f0c2 wallet: Remove left-over BIP70 comment (Fabian Jahr)
Pull request description:
A small follow-up to #17165 which removed BIP70 support.
1. Removes one leftover mention of BIP70 in a comment.
2. Removes BIP70 reference in comments on network/chain name strings. These can be removed as they are not really helpful and also incorrect: BIP70 only defines "main" and "test" but not "regtest". If/When signet gets merged we will add another name to the list that is not defined in BIP70. Mostly there is also an exhaustive list of the options included in the comment anyway.
If we would like to keep an identifier for this naming scheme, I would suggest switching to something more generic, like 'short chain name'. Happy to implement that if that is preferred. Alternatively, we could add a reference to `CBaseChainParams`. That would also mean we don't have to change these lines again for signet.
ACKs for top commit:
MarcoFalke:
ACK 3ed8e3d079
Tree-SHA512: 9a7c0b9cacbb67bd31a089ffdc6f1ebc7f336493e2c8266eb697da34dce2b505a431d5639a3e4fc34f9287361343e861b55dc2662e0a1d2095cc1046db77d6ee
a35b6824f3 Add assertion to randrange that input is not 0 (Jeremy Rubin)
Pull request description:
From the comment in randrange, their is an implicit argument that randrange cannot accept an argument of 0. If the argument is 0, then we have to return {}, which is not possible in a uint64_t.
The current code takes a very interesting approach, which is to return [0..std::numeric_limits<uint64_t>]. This can cause all sorts of fun problems, like allocating a lot of memory, accessing random memory (maybe with your private keys), and crashing the computer entirely.
This gives us three choices of how to make it "safe":
1) return Optional<uint64_t>
2) Change the return type to [0..range]
3) Return 0 if 0
4) Assert(range)
So which solution is best?
1) seems a bit overkill, as it makes any code using randrange worse.
2) Changing the return type as in 2 could be acceptable, but it imposes the potential overflow checking on the caller (which is what we want).
3) An interesting option -- effective makes the return type in {0} U [0..range]. But this is a bad choice, because it leads to code like `vec[randrange(vec.size())]`, which is incorrect for an empty vector. Null set should mean null set.
4) Assert(range) stands out as the best mitigation for now, with perhaps a future change to solution 2. It prevents the error from propagating at the earliest possible time, so the program crashes cleanly rather than by freezing the computer or accessing random memory.
ACKs for top commit:
instagibbs:
Seems reasonable for now, ACK a35b6824f3
laanwj:
ACK a35b6824f3
promag:
ACK a35b6824f3.
Tree-SHA512: 8fc626cde4b04b918100cb7af28753f25ec697bd077ce0e0c640be0357626322aeea233e3c8fd964ba1564b0fda830b7f5188310ebbb119c113513a4b89952dc
ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank
wallet flag. Just make that it's own function and not expose the flag
writing directly.
This does not change behavior.
The default (i.e., generated by a compiler) copy constructor does the
same things.
Also this prevents -Wdeprecated-copy warning for implicitly declared
operator= in GCC 9.
b0c774b48a Add new mempool benchmarks for a complex pool (Jeremy Rubin)
Pull request description:
This PR is related to #17268.
It adds a mempool stress test which makes a really big complicated tx graph, and then, similar to mempool_eviction test, trims the size.
The test setup is to make 100 original transactions with Rand(10)+2 outputs each.
Then, 800 times:
we create a new transaction with Rand(10) + 1 parents that are randomly sampled from all existing transactions (with unspent outputs). From each such parent, we then select Rand(remaining outputs) +1 50% of the time, or 1 outputs 50% of the time.
Then, we trim the size to 3/4. Then we trim it to just a single transaction.
This creates, hopefully, a big bundle of transactions with lots of complex structure, that should really put a strain on the mempool graph algorithms.
This ends up testing both the descendant and ancestor tracking.
I don't love that the test is "unstable". That is, in order to compare this test to another, you really can't modify any of the internal state because it will have a different order of invocations of the deterministic randomness. However, it certainly suffices for comparing branches.
Top commit has no ACKs.
Tree-SHA512: cabe96b849b9885878e20eec558915e921d49e6ed1e4b011b22ca191b4c99aa28930a8b963784c9adf78cc8b034a655513f7a0da865e280a1214ae15ebb1d574
5710dadf9b test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)
Pull request description:
Cleans up #15140 which fixes commit 6b25f29a91 where opcodes were lost in translation.
ACKs for top commit:
laanwj:
code review ACK 5710dadf9b
Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
8734c856f8 Replace the LogPrint function with a macro (Jeffrey Czyz)
Pull request description:
Calling `LogPrint` with a category that is not enabled results in
evaluating the remaining function arguments, which may be arbitrarily
complex (and possibly expensive) expressions. Defining `LogPrint` as a
macro prevents this unnecessary expression evaluation.
This is a partial revert of #14209. The decision to revert is discussed
in #16688, which adds verbose logging for validation event notification.
ACKs for top commit:
jnewbery:
ACK 8734c856f8
Tree-SHA512: 19e995eaef0ff008a9f8c1fd6f3882f1fbf6794dd7e2dcf5c68056be787eee198d2956037d4ffba2b01e7658b47eba276cd7132feede78832373b3304203961e
a8b82867d5 Fix incorrect help-debug for -checkpoints (Antoine Riard)
Pull request description:
ACKs for top commit:
jnewbery:
ACK a8b82867d5 for improving the `-prune` help text.
MarcoFalke:
ACK a8b82867d5
Tree-SHA512: 973fa97436be09a9939386dc00023420a7296a9e268356bf26aa06468f9f0d2c822205a4f1ce8f44a0562aa64ad90a43dec5697af656ef28ba6829e4e4360e94
9cae3d5e94 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)
Pull request description:
The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.
This is a follow-up to #17235 which accidentally broke those two fuzzers.
Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.
Top commit has no ACKs.
Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
d314e8a818 refactor: Replace all uses of boost::optional with our own Optional type (Wladimir J. van der Laan)
Pull request description:
Replace all uses of boost::optional with our own Optional type. Luckily, there aren't so many.
After this:
- `boost::optional` is no longer used directly (only through `Optional` which is an alias for it)
- `boost/optional.hpp` is only included in one place
ACKs for top commit:
MarcoFalke:
ACK d314e8a818
practicalswift:
ACK d314e8a818 -- diff looks correct + satisfying to see incremental progress towards the goal of a Boost free future :)
jtimon:
ACK d314e8a818
fanquake:
ACK d314e8a818
Tree-SHA512: b43e0017af81b07b5851377cd09624f114510ac5b9018d037664b58ad0fc8e893e30946b61f8f5e21e39125925bf9998a81f2226b468aab2df653ee57ed3213d
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.
Places pindexBestHeader is used:
* Various GUI displays of the best header and getblockchaininfo["headers"],
I don't think changing this is bad, and if anything this is less confusing
in the presence of an invalid block.
* IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
isn't some crazy invalid chain is better than the alternative, even in the
case where you are rejecting the current chain due to hardware error (since
hopefully in that case you won't get any new blocks anyway).
* ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
block we're connecting leads to something with nMinimumChainWork (preventing
a user-set assumevalid from having bogus work) and that the block we're
connecting leads to pindexBestHeader (I'm not too worried about this one -
it's nice to "disable" assumevalid if we have a long invalid headers chain,
but I don't see it as a critical protection).
* BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
requested block is within a month of the "current chain". I don't think this
is a meaningful difference, if we're rejecting the current tip we're
trivially fingerprintable anyway, and if the chain really does have a bunch
of invalid crap near the tip, using the best not-invalid header is likely a
better criteria.
* ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
of whether a block request is "historical" for the purpose of bandwidth
limiting. Similarly, I don't see why this is a meaningful change.
* We use pindexBestHeader for requesting missing headers on receipt of a
headers/compact block message or block inv as well as for initial getheaders.
I think this is definitely wrong, using the best not-invalid header for such
requests is much better.
* We use pindexBestHeader to define the "current chain" for deciding when
we're close to done with initial headers sync. I don't think this is a
meaningful change.
* We use pindexBestHeader to decide if initial headers sync has timed out. If
we're rejecting the chain due to hardware error this may result in
additional cases where we ban a peer, but this is already true, so I think
its fine.
