152e8baf08 Use salted hasher instead of nonce in sigcache (Jeremy Rubin)
5495fa5850 Add Hash Padding Microbenchmarks (Jeremy Rubin)
Pull request description:
This PR replaces nonces in two places with pre-salted hashers.
The nonce is chosen to be 64 bytes long so that it forces the SHA256 hasher to process the chunk. This leaves the next 64 (or 56 depending if final chunk) open for data. In the case of the script execution cache, this does not make a big performance improvement because the nonce was already properly padded to fit into one buffer, but does make the code a little simpler. In the case of the sig cache, this should reduce the hashing overhead slightly because we are less likely to need an additional processing step.
I haven't benchmarked this, but back of the envelope it should reduce the hashing by one buffer for all combinations except compressed public keys with compact signatures.
ACKs for top commit:
ryanofsky:
Code review ACK 152e8baf08. No code changes, just rebase since last review and expanded commit message
Tree-SHA512: b133e902fd595cfe3b54ad8814b823f4d132cb2c358c89158842ae27daee56ab5f70cde2585078deb46f77a6e7b35b4cc6bba47b65302b7befc2cff254bad93d
f9ee0f37c2 Add comments to CustomUintFormatter (Pieter Wuille)
4eb5643e35 Convert everything except wallet/qt to new serialization (Pieter Wuille)
2b1f85e8c5 Convert blockencodings_tests to new serialization (Pieter Wuille)
73747afbbe Convert merkleblock to new serialization (Pieter Wuille)
d06fedd1bc Add SER_READ and SER_WRITE for read/write-dependent statements (Russell Yanofsky)
6f9a1e5ad0 Extend CustomUintFormatter to support enums (Russell Yanofsky)
769ee5fa00 Merge BigEndian functionality into CustomUintFormatter (Pieter Wuille)
Pull request description:
The next step of changes from #10785.
This:
* Adds support for enum serialization to `CustomUintFormatter`, used in `CAddress` for service flags.
* Merges `BigEndian` into `CustomUintFormatter`, used in `CNetAddr` for port numbers.
* Converts everything (except wallet and gui) to use the new serialization framework.
ACKs for top commit:
MarcoFalke:
re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter 📂
ryanofsky:
Code review ACK f9ee0f37c2. Just new commit adding comment since last review
jonatack:
Code review re-ACK f9ee0f37c2 only change since last review is an additional commit adding Doxygen documentation for `CustomUintFormatter`.
Tree-SHA512: e7a0a36afae592d5a4ff8c81ae04d858ac409388e361f2bc197d9a78abca45134218497ab2dfd6d031e0cce0ca586cf857077b7c6ce17fccf67e2d367c1b6cd4
fabe44e815 bench: Start nodes with -nodebuglogfile (MarcoFalke)
Pull request description:
For benchmarking we don't want to depend on the speed of the disk or the amount of debug logging
ACKs for top commit:
fanquake:
ACK fabe44e815 - This makes some of these benchmarks significantly faster to run. MempoolEviction total runtime is down from ~46s to 11s on my machine:
Tree-SHA512: d99700901650325896b9115d20b84a27042152f46266f595bf7ea1414528c0b346f4e707a12ee8b8ba99c35cf155e645e67971c1b2a679c4e609c400ff8b08ae
fa4632c417 test: Move boost/stdlib includes last (MarcoFalke)
fa488f131f scripted-diff: Bump copyright headers (MarcoFalke)
fac5c37300 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c417
jonatack:
ACK fa4632c417, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
Currently it's possible for ReleaseWallet to delete the CWallet pointer while
it is processing BlockConnected, etc chain notifications.
To fix this, unregister from notifications earlier in UnloadWallet instead of
ReleaseWallet, and use a new RegisterSharedValidationInterface function to
prevent the CValidationInterface shared_ptr from being deleted until the last
notification is actually finished.
e6e622e5a0 Implement O(1) OP_IF/NOTIF/ELSE/ENDIF logic (Pieter Wuille)
d0e8f4d5d8 [refactor] interpreter: define interface for vfExec (Anthony Towns)
89fb241c54 Benchmark script verification with 100 nested IFs (Pieter Wuille)
Pull request description:
While investigating what mechanisms are possible to maximize the per-opcode verification cost of scripts, I noticed that the logic for determining whether a particular opcode is to be executed is O(n) in the nesting depth. This issue was also pointed out by Sergio Demian Lerner in https://bitslog.wordpress.com/2017/04/17/new-quadratic-delays-in-bitcoin-scripts/, and this PR implements a variant of the O(1) algorithm suggested there.
This is not a problem currently, because even with a nesting depth of 100 (the maximum possible right now due to the 201 ops limit), the slowdown caused by this on my machine is around 70 ns per opcode (or 0.25 s per block) at worst, far lower than what is possible with other opcodes.
This PR mostly serves as a proof of concept that it's possible to avoid it, which may be relevant in discussions around increasing the opcode limits in future script versions. Without it, the execution time of scripts can grow quadratically with the nesting depth, which very quickly becomes unreasonable.
This improves upon #14245 by completely removing the `vfExec` vector.
ACKs for top commit:
jnewbery:
Code review ACK e6e622e5a0
MarcoFalke:
ACK e6e622e5a0🐴
fjahr:
ACK e6e622e5a0
ajtowns:
ACK e6e622e5a0
laanwj:
concept and code review ACK e6e622e5a0
jonatack:
ACK e6e622e5a0 code review, build, benches, fuzzing
Tree-SHA512: 1dcfac3411ff04773de461959298a177f951cb5f706caa2734073bcec62224d7cd103767cfeef85cd129813e70c14c74fa8f1e38e4da70ec38a0f615aab1f7f7
fae86c38bc util: Remove unused MilliSleep (MarcoFalke)
fa9af06d91 scripted-diff: Replace MilliSleep with UninterruptibleSleep (MarcoFalke)
fa4620be78 util: Add UnintrruptibleSleep (MarcoFalke)
Pull request description:
We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in `std::thread`
ACKs for top commit:
ajtowns:
ACK fae86c38bc quick code review
practicalswift:
ACK fae86c38bc -- patch looks correct
sipa:
Concept and code review ACK fae86c38bc
fanquake:
ACK fae86c38bc - note that an instance of `DHAVE_WORKING_BOOST_SLEEP_FOR` was missed in the [linter](https://github.com/bitcoin/bitcoin/blob/master/test/lint/extended-lint-cppcheck.sh#L69), but that can be cleaned up later.
Tree-SHA512: 7c0f8eb197664b9f7d9fe6c472c77d384f11c797c913afc31de4b532e3b4fd9ea6dd174f92062ff9d1ec39b25e0900ca7c597435add87f0f2477d9557204848c
The only difference between SetupDummyInputs() in test/transaction_tests.cpp
and the one in bench/ccoins_caching.cpp was the nValue amounts of the outputs,
so we allow to pass those in an extra (fixed-size) array parameter.
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
3f373659d7 Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c403 Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa59e Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd0a3 Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30d59 refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcccd3 HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511206 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e846 Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5538 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610eddbc List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3ee5 Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08ad94 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479694 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Continuation of wallet boxes project.
Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.
***
Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign.
There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s.
The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script.
Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed.
This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).
ACKs for top commit:
instagibbs:
re-utACK 3f373659d7
Sjors:
re-utACK 3f373659d7 (it still compiles on macOS after https://github.com/bitcoin/bitcoin/pull/17261#discussion_r370377070)
meshcollider:
Tested re-ACK 3f373659d7
Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
faa92a2297 rpc: Remove mempool global from miner (MarcoFalke)
6666ef13f1 test: Properly document blockinfo size in miner_tests (MarcoFalke)
Pull request description:
The miner needs read-only access to the mempool. Instead of using the mutable global `::mempool`, keep a immutable reference to a mempool that is passed to the miner. Apart from the obvious benefits of removing a global and making things immutable, this might also simplify testing with multiple mempools.
ACKs for top commit:
promag:
ACK faa92a2297.
fjahr:
ACK faa92a2297
jnewbery:
Code review ACK faa92a2297
Tree-SHA512: c44027b5d2217a724791166f3f3112c45110ac1dbb37bdae27148a0657e0d1a1d043b0d24e49fd45465ec014224d1b7eb15c92a33069ad883fa8ffeadc24735b
78e283e656 [test] move wallet helper functions into test library (Martin Zumsande)
f613e5dfda [test] move mining helper functions into test library (Martin Zumsande)
2cb4e8bdc7 [test] move string helper functions into test library (Martin Zumsande)
Pull request description:
This disbands `test/util.h` and `test/util.cpp` and moves the content into the test utility library recently created in #17542, so that all test utility functions are in one place.
The content of the original files are split into three modules:
1) string helper functions go to `test/util/str`
2) mining helper functions go to the newly created `test/util/mining`
3) wallet helper functions go to the newly created `test/util/wallet`
ACKs for top commit:
MarcoFalke:
ACK 78e283e656🔧
Tree-SHA512: f182a61e86e76c32bcb84e37f44904d3a4a9c5a321f7a8efdda5368a6623cb8b5a5384ec4f96e67f0357b0c22099f6e3ecd0ac4cb467e3fa3f3128f8d36edfb8
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
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
3004d5a12d [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5b [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b31 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e492 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d
amitiuttarwar:
code review ACK 3004d5a12d. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
362ded410b Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b7 scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5 Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e scripted-diff: Rename InitInterfaces to NodeContext (Russell Yanofsky)
Pull request description:
This change is mainly a naming / organization change intended to simplify #10102. It:
- Renames struct InitInterfaces to struct NodeContext and moves it from
src/init.h to src/node/context.h. This is a cosmetic change intended to make
the point of the struct more obvious.
- Gets rid of BanMan and ConnMan globals making them NodeContext members
instead. Getting rid of these globals has been talked about in past as a way
to implement testing and simulations. Making them NodeContext members is a
way of keeping them accessible without the globals.
- Splits g_rpc_interfaces global into g_rpc_node and g_rpc_chain globals. This
better separates node and wallet rpc methods. Node RPC methods should have
access NodeContext, while wallet RPC methods should only have indirect access
to node functionality via interfaces::Chain.
- Adds NodeContext& references to interfaces::Chain class and the
interfaces::MakeChain() function. This is needed to access ConnMan and BanMan
instances without the globals.
- Gets rid of redundant Node and Chain instances in Qt tests. This is
needed due to the previous MakeChain change, and also makes test setup a
little more straightforward. More cleanup could be done in the future, but it
will require deduplication of bitcoind, bitcoin-qt, and TestingSetup init
code.
ACKs for top commit:
laanwj:
ACK 362ded410b
Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2
c72906dcc1 refactor: Remove redundant c_str() calls in formatting (Wladimir J. van der Laan)
Pull request description:
Our formatter, tinyformat, *never* needs `c_str()` for strings. Still, many places call it redundantly, resulting in longer code and a slight overhead.
Remove redundant `c_str()` calls for:
- `strprintf`
- `LogPrintf`
- `tfm::format`
(also, combined with #17095, I think this improves logging in case of unexpected embedded NULL characters)
ACKs for top commit:
ryanofsky:
Code review ACK c72906dcc1. Easy to review with `git log -p -n1 --word-diff-regex=. -U0 c72906dcc11a73fa06a0adf97557fa756b551bee`
Tree-SHA512: 9e21e7bed8aaff59b8b8aa11571396ddc265fb29608c2545b1fcdbbb36d65b37eb361db6688dd36035eab0c110f8de255375cfda50df3d9d7708bc092f67fefc
