| Age | Commit message (Collapse) | Author |
|---|
|
fa538813b1c382cf135cbf2a0cc3fa01f36964d8 scripted-diff: Replace ::mempool with m_node.mempool in tests (MarcoFalke)
8888ad02e204b0fa7a2ea2cfed2fc3f298cf1623 test: Replace recursive lock with locking annotations (MarcoFalke)
fac07f2038a3ccd5edadc6e6122c02fa30e697bd node: Add reference to mempool in NodeContext (MarcoFalke)
Pull request description:
This is the first step toward making the mempool a global that is not initialized before main.
#### Motivation
Currently the mempool is a global that is initialized before the `main` function. This is confusing and easy to get wrong. E.g. the mempool constructor queries state that has not been initialized, like randomness (fixed), or command line arguments (not an issue last time I checked). Also without having the chainstate (chain tip) initialized first, it doesn't make conceptually sense to have a mempool, since the mempool builds txs on top of the utxo set (chain tip).
Finally, in the future someone might want to run a consensus-only full node (`-nowallet -noblockfilter -no... -nomempool` command line options) that only verifies blocks and updates the utxo set.
This is conceptually the same change that has already been done for the connection manager `CConnman`.
ACKs for top commit:
jnewbery:
utACK fa538813b1c382cf135cbf2a0cc3fa01f36964d8
ariard:
Tested ACK fa53881.
Tree-SHA512: 2c446a8a51476354aad7126c2b833500d36b24490caa94f847b2bdc622054de0dae28980f23e3d91b1b492dc32931656d98dbd019af9e4e58f2f8c5375aac694
|
|
49f4c7f0699e5e19ac6e41ef5b607392dd7a2983 tests: Add fuzzing harness for various PSBT related functions (practicalswift)
Pull request description:
Add fuzzing harness for various PSBT related functions.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/psbt
```
ACKs for top commit:
MarcoFalke:
re-ACK 49f4c7f0699e5e19ac6e41ef5b607392dd7a2983 🐟
Tree-SHA512: 4cebe62bd8c244ee40a43e829f5bd175ab40e1dfbbab1affb1529374858225820d6c9fa9ba45862bf56c1522845422fd96d620cedbdec52a67ac1449dec4e1b2
|
|
|
|
-BEGIN VERIFY SCRIPT-
# tx pool member access (mempool followed by dot)
sed --regexp-extended -i -e 's/(::)?\<mempool\>\.([a-zA-Z])/m_node.mempool->\2/g' $(git grep -l mempool ./src/test)
# plain global (mempool not preceeded by dot, but followed by comma)
sed --regexp-extended -i -e 's/([^\.])(::)?\<mempool\>,/\1*m_node.mempool,/g' $(git grep -l mempool ./src/test)
-END VERIFY SCRIPT-
|
|
Also, use m_node.mempool instead of the global
|
|
Currently it is an alias to the global ::mempool and should be used as
follows.
* Node code (validation and transaction relay) can use either ::mempool
or node.mempool, whichever seems a better fit.
* RPC code should use the added convenience getter EnsureMempool, which
makes sure the mempool exists before use. This prepares the RPC code
to a future where the mempool might be disabled at runtime or compile
time.
* Test code should use m_node.mempool directly, as the mempool is always
initialized for tests.
|
|
The function IsStandardTx() returns rejection reason "scriptsig-size" if any
one the inputs' scriptSig is larger than 1650 bytes.
|
|
|
|
083c954b02a4e7d0708349eeaf3bac2b5947fb0e Add settings_tests (Russell Yanofsky)
7f40528cd50fc43ac0bd3e785de24d661adddb7a Deduplicate settings merge code (Russell Yanofsky)
9dcb952fe5f85529ab28e091af7534e72c21c90f Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cfe8af088bd8ea884be2f79f0f3cac555d5 Remove includeconf nested scope (Russell Yanofsky)
5a84aa880f6da0bac0e2144733fdef3b8558c761 Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e75487461ec9bff433144f0db831b682403 Clarify emptyIncludeConf logic (Russell Yanofsky)
Pull request description:
This is a refactoring-only change that makes it easier to add a new settings source.
This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.
This change:
- Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935).
- Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways.
- Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108).
The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended:
* 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935 – _Add \<datadir>/settings.json persistent settings storage_
* 04c80c40df9fc6f4734ba238ea7f65607cf88089 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_
ACKs for top commit:
ariard:
ACK 083c954
jnewbery:
ACK 083c954b02a4e7d0708349eeaf3bac2b5947fb0e
jamesob:
ACK 083c954b02a4e7d0708349eeaf3bac2b5947fb0e
Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
|
|
36b68de5b2938722911db900ca299f7008780d01 Remove getBlockDepth method from Chain::interface (Antoine Riard)
b66c429c56c85fa16c309be0b2bca9c25fdd3e1a Remove locked_chain from GetDepthInMainChain and its callers (Antoine Riard)
0ff03871add000f8b4d8f82aeb168eed2fc9dc5f Use CWallet::m_last_block_processed_height in GetDepthInMainChain (Antoine Riard)
f77b1de16feee097a88e99d2ecdd4d84beb4f915 Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (Antoine Riard)
769ff05e48fb53d4b62c59060424a0fea71d0aab Refactor some importprunedfunds checks with guard clause (Antoine Riard)
5971d3848e09abf571e5308185275296127efca4 Add block_height field in struct Confirmation (Antoine Riard)
9700fcb47feca9d78e005b8d18b41148c8f6b25f Replace CWalletTx::SetConf by Confirmation initialization list (Antoine Riard)
5aacc3eff15b9b5bdc951f1e274f00d581f63bce Add m_last_block_processed_height field in CWallet (Antoine Riard)
10b4729e33f76092bd8cfa06d1a5e0a066436f76 Pass block height in Chain::BlockConnected/Chain::BlockDisconnected (Antoine Riard)
Pull request description:
Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.
I think it's ready for a first round of review before to get further.
- `BlockUntilSyncedToCurrent` : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.
~~- `AbandonTransaction` : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn't conflicted anymore so we return 0 as tx is again unconfirmed~~ After #16624, we instead rely on Confirmation.
~~- `AddToWalletIfInvolvingMe`: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in `mapWallet` with a height set to zero so we remove check on block_hash.IsNull~~ Already done in #16624
ACKs for top commit:
jnewbery:
@jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de5b2938722911db900ca299f7008780d01).
jkczyz:
> @jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch ([36b68de](https://github.com/bitcoin/bitcoin/commit/36b68de5b2938722911db900ca299f7008780d01)).
meshcollider:
utACK 36b68de5b2938722911db900ca299f7008780d01
ryanofsky:
Code review ACK 36b68de5b2938722911db900ca299f7008780d01. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment
jnewbery:
utACK 36b68de5b2938722911db900ca299f7008780d01
promag:
Code review ACK 36b68de5b2938722911db900ca299f7008780d01.
Tree-SHA512: 08b89a0bcc39f67c82a6cb6aee195e6a11697770c788ba737b90986b4893f44e90d1ab9ef87239ea3766508b7e24ea882b7199df41173ab27a3d000328c14644
|
|
Co-authored-by: James O'Beirne <james.obeirne@gmail.com>
|
|
Get rid of settings merging code in util/system.cpp repeated 5 places,
inconsistently:
- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs
Having settings merging code separated from parsing simplifies parsing somewhat
(for example negated values can simply be represented as false values instead
of partially cleared or emply placeholder lists).
Having settings merge happen one place instead of 5 makes it easier to add new
settings sources and harder to introduce new inconsistencies in the way
settings are merged.
This commit does not change behavior in any way.
|
|
fae43a97ca947cd0802392e9bb86d9d0572c0fba test: Seed test RNG context for each test case, print seed (MarcoFalke)
Pull request description:
Debugging failing unit tests is hard if the failure is non-deterministic and the seed is not known.
Fix that by printing the seed and making it possible to set the seed from outside.
ACKs for top commit:
davereikher:
Tested ACK fae43a97ca947cd0802392e9bb86d9d0572c0fba
Tree-SHA512: 33d848dd1f4180d3664ecf60e9810c2a93590c05276b2c46b1e4fe6e376b45916a46b90c803bb602750ab666da3a05ce499e550024685a90b8cc38fab6667cb8
|
|
5506ecfe7a65d5705616bc048f2f1735b89993fb [refactor] Replace global int nScriptCheckThreads with bool (John Newbery)
d9957623b48a7c3eff0ac750d1245fabfb1843a2 [tests] Don't use TestingSetup in the checkqueue_tests (John Newbery)
Pull request description:
The meaning of this value is confusing. Refactor it and add comments.
ACKs for top commit:
sipa:
ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb
promag:
ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb, only change was addressing my nits.
laanwj:
Code review ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb
MarcoFalke:
ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb 🥐
Tree-SHA512: 78536727c98d2c23f3c0f3f169131474fef9a4486ae65029011caf06eab30f6f70ff73a65b2fb04a5d969fc1150858d1c6ea4767f04d48c1eea6b829316d0e63
|
|
fa4c6fa9b1139791f45f1495d662c1c7cd2f7ed6 doc: Add documentation for new test/lib (MarcoFalke)
faec28252cf4f8e754c689a7a44fd421f631db50 scripted-diff: test: Move setup_common to test library (MarcoFalke)
Pull request description:
Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit.
Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes it. For example, setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui)
ACKs for top commit:
practicalswift:
ACK fa4c6fa9b1139791f45f1495d662c1c7cd2f7ed6 -- diff looks correct and Travis is happy
jonatack:
ACK fa4c6fa9b1139791f45f1495d662c1c7cd2f7ed6 with the reserve that the commit messages (and PR description) contain the motivation for this change. Built, ran tests, light code review.
ryanofsky:
Code review ACK fa4c6fa9b1139791f45f1495d662c1c7cd2f7ed6. I didn't realize `lib` was actually name of existing directory, not a new name. But in any case this looks good and nice to have one scripted diff instead of two.
Tree-SHA512: 2e176df90c60578276e4a6dc83ff57ff59d8e666ecf30c5ceacb8c326725da91baa4cac3dfa7a2e1605f58122a3e3e27e4938ff33e3a0ce7ea53afffebbf57a4
|
|
The global nScriptCheckThreads int is confusing and is only needed for
its int-ness in AppInitMain. Move all `-par` parsing logic there and
replace the int nScriptCheckThreads with a bool
g_parallel_script_checks.
Also tidy up logic and improve comments.
|
|
It's only needed for a hardcoded int, which we can define locally.
|
|
|
|
-BEGIN VERIFY SCRIPT-
# Move files
for f in $(git ls-files src/test/lib/); do git mv $f src/test/util/; done
git mv src/test/setup_common.cpp src/test/util/
git mv src/test/setup_common.h src/test/util/
# Replace Windows paths
sed -i -e 's|\\setup_common|\\util\\setup_common|g' $(git grep -l '\\setup_common')
sed -i -e 's|src\\test\\lib\\|src\\test\\util\\|g' build_msvc/test_bitcoin/test_bitcoin.vcxproj
# Everything else
sed -i -e 's|/setup_common|/util/setup_common|g' $(git grep -l 'setup_common')
sed -i -e 's|test/lib/|test/util/|g' $(git grep -l 'test/lib/')
# Fix include guard
sed -i -e 's|BITCOIN_TEST_SETUP_COMMON_H|BITCOIN_TEST_UTIL_SETUP_COMMON_H|g' ./src/test/util/setup_common.h
sed -i -e 's|BITCOIN_TEST_LIB_|BITCOIN_TEST_UTIL_|g' $(git grep -l 'BITCOIN_TEST_LIB_')
-END VERIFY SCRIPT-
|
|
286f197704e82045c762d332aba5d1ac52e0212d Add util_ArgParsing test (Russell Yanofsky)
Pull request description:
ArgsManager test coverage for parsing of integer and boolean values is
currently very poor and doesn't give us a way of knowing whether changes to
ArgsManager may unintentionally break backwards compatibility, so this adds a
new test to catch regressions.
ACKs for top commit:
promag:
ACK 286f197, more surprising results 😱
laanwj:
ACK 286f197704e82045c762d332aba5d1ac52e0212d
Tree-SHA512: 9e1db3ef87e55abbc280af60c088f35765a1f9e2ec20507ad0c1992027b875490016868dcb8cc287e6df279dd0e00f10550901af3de3d36287867249e0bd8207
|
|
3645e4ca0033bb6365f41ef710111780c139370f Add missing newline in util_ChainMerge test (Russell Yanofsky)
Pull request description:
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
laanwj:
ACK 3645e4ca0033bb6365f41ef710111780c139370f
practicalswift:
ACK 3645e4ca0033bb6365f41ef710111780c139370f -- diff looks correct
Tree-SHA512: ca5bde9b9f553811d4827113f4880d15d7b8f4f1455b95bbf34c9a1512fdd53062f1a2133c50d9b54f94160a1ee77a54bc82681a5f3bf25d2b0d01f8a8e95165
|
|
ArgsManager test coverage for parsing of integer and boolean values is
currently very poor and doesn't give us a way of knowing whether changes to
ArgsManager may unintentionally break backwards compatibility, so this adds a
new test to catch regressions.
|
|
dcef9a2922317cb2849f397366b6c56d755db6c9 logs: add timing information to FlushStateToDisk() (James O'Beirne)
41edaf227a69bc4846d5996675e8763fdfe0f367 logs: add BCLog::Timer and related macros (James O'Beirne)
Pull request description:
It's currently annoying to detect FlushStateToDisk() calls when benchmarking since they have to be inferred from a drop in coins count from the `UpdateTip: ` log messages. This adds a new logging utility, `BCLog::Timer`, and some related macros that are generally useful for printing timing-related logging messages, and a message that is unconditionally written when the coins cache is flushed to disk.
```
2019-09-04T20:17:51Z FlushStateToDisk: write block and undo data to disk completed (3ms)
2019-09-04T20:17:51Z FlushStateToDisk: write block index to disk completed (370ms)
2019-09-04T20:17:51Z FlushStateToDisk: write coins cache to disk (2068451 coins, 294967kB) completed (21481ms)
```
ACKs for top commit:
laanwj:
Thanks, ACK dcef9a2922317cb2849f397366b6c56d755db6c9
ryanofsky:
Code review ACK dcef9a2922317cb2849f397366b6c56d755db6c9. No changes since last review other than moving code to new timer.h header
Tree-SHA512: 6d61e48a062d3edb48d0e056a6f0b1f8031773cc99289ee4544f8349d24526b88519e1e304009d56e428f1eaf76c857bf8e7e1c0b6873a6f270306accb5edc3d
|
|
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
|
|
fa2c44c3ccc3e7a54e2afc862addd555d5a6e51b test: Add ASSERT_DEBUG_LOG to unit test framework (MarcoFalke)
fa1936f57bbf5aebb1f8fc18701441d79219d443 logging: Add member for arbitrary print callbacks (MarcoFalke)
Pull request description:
Similar to `assert_debug_log` in the functional test framework
Top commit has no ACKs.
Tree-SHA512: aa9eaeca386b61d806867c04a33275f6eb4624fa5bf50f2928d16c83f5634bac96bcac46f9e8eda3b00b4251c5f12d7b01d6ffd84ba8e05c09eeec810cc31251
|
|
b2ff500fb3e4fa05de366ab1900825bea1f70377 test: add "diamond" unit test to MempoolAncestryTests (Sebastian Falbesoner)
Pull request description:
Approaches #17271 (_Missing Unit Test for Ancestors "diamond"_).
If ancestors are represented more than once (in this case `ta` and `tb`), check that those are not overcounted.
ACKs for top commit:
laanwj:
ACK b2ff500fb3e4fa05de366ab1900825bea1f70377
Tree-SHA512: 82a6573cc7f0e82bf6fcfe207d7ddecbf297d2a203d22e95b73d887e3cb280f45a3c5f649161561c1be1eb560ff81b9b385868f205d1c12284211c2377e5ad99
|
|
To do so we update CValidationInterface::BlockDisconnect to take a
CBlockIndex pointing to the block being disconnected.
This new parameter will be use in the following commit to establish
wallet height.
|
|
b7541705d0abfbddf682a0134f3fa8a8e1d06cdf tests: Add fuzzing harness for Bech32 encoding/decoding (practicalswift)
85a34b1683233060c4500cfb8e3f5d84c8b8f0da tests: Move CaseInsensitiveEqual to test/util/str (practicalswift)
Pull request description:
Add fuzzing harness for Bech32 encoding/decoding.
**Testing this PR**
Run:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/bech32 -max_total_time=60
…
```
ACKs for top commit:
jonatack:
ACK b7541705d0abfbddf682a0134f3fa8a8e1d06cdf
Tree-SHA512: ade01d30c6886a083b806dbfff08999cc0d08e687701c670c895e261ed242c789e8a0062d4ebbe8f82676b8f168dc37e83351a88822c9c0eab478572a9e1ec02
|
|
1a8f0d5a74d5cc0000456932babf35301f5c1686 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de630354fc6808b9b13b9e82da1a82f2f50f26a [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)
Pull request description:
Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.
Needed for https://github.com/bitcoin/bitcoin/pull/16698.
ACKs for top commit:
ajtowns:
ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686
MarcoFalke:
re-ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686
naumenkogs:
ACK 1a8f0d5, and let's merge it and come back to it later.
Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
|
|
|
|
|
|
|
|
Makes logging timing information about a block of code easier.
|
|
|
|
|
|
Approaches #17271.
If ancestors are represented more than once, check that those are not
overcounted.
|
|
fa0a731d007a8e76d2740a2e6ead2289de77e475 test: Add RegTestingSetup to setup_common (MarcoFalke)
fa54b3e2485f701aa420c37f09a2859a5b805161 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 fa0a731d007a8e76d2740a2e6ead2289de77e475 -- diff looks correct
Tree-SHA512: 02b9765580b355ed8d1be555f8ae11fa6e3d575f5cb177bbdda0319378837e29de5555c126c477dc8a1e8a5be47335afdcff152cf2dea2fbdd1a988ddde3689b
|
|
5710dadf9b282524fddf42c682351cd8022ed7bf 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 5710dadf9b282524fddf42c682351cd8022ed7bf
Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
|
|
|
|
ECCVerifyHandle dependency
9cae3d5e94f4481e0d251c924314e57187a07a60 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
|
|
|
|
|
|
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [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 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
|
|
595cc9bcafb16709c0dd4a36d5921feb009ce430 docs: Add undefined to --with-sanitizers=fuzzer,address (practicalswift)
d5dbb4898c437d3d2a5798d79c3986ba9e0b72f9 tests: Add fuzzing harness for ISO-8601 related functions (practicalswift)
Pull request description:
Add fuzzing harness for ISO-8601 related functions.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/parse_iso8601
…
```
Top commit has no ACKs.
Tree-SHA512: 8d4ad9e4eef546e97ea330cf518fdd7241c6f016d6c45c011369d5cdd832bbbc3564d1a990c953ffb33b0c05e58f5533e7b6fd77062f8484df36da1513567915
|
|
|
|
362ded410b8cb1104b7ef31ff8488fec4824a7d5 Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b751a3e6b3b9f6fb7961c442877fb65a scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5e42feaf7bfa5f41e80292aaa73cd7d Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b71c9d91399c31b043237091be2e5e7 MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e6765b185bd55f4c541f9e27aeea29d 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 362ded410b8cb1104b7ef31ff8488fec4824a7d5
Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2
|
|
c1c6c410a66996b2d60d5172189b5a5ec8100842 test: add reason checks for non-standard txs in test_IsStandard (Sebastian Falbesoner)
Pull request description:
While taking a look at #17272 I noticed that for some reason the unit test `test_IsStandard` (which was not adapted to the policy change in the referenced PR commits) didn't fail as expected:
https://github.com/bitcoin/bitcoin/blob/6a97e8a060f7632bbaee27d3de8035dc6ebe3895/src/test/transaction_tests.cpp#L758-L762
It turned out that `IsStandardTx()` returned `"dust"` as rejection reason (instead of the expected `"multi-op-return"`), leading to the conclusion that https://github.com/bitcoin/bitcoin/pull/17272/commits/5fe6f052bd37a16b2849e05f5cf18d7e194bc705 erroneously performs the `IsDust()` check also for TX_NULL_DATA transactions. To avoid cases like this in the future, this PR makes the unit test `test_IsStandard` more strict by also checking for the concrete reason after each occurence of `IsStandardTx()` returning false.
ACKs for top commit:
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/17299/commits/c1c6c410a66996b2d60d5172189b5a5ec8100842
Tree-SHA512: c7419884cc52977c73f8f8c476eaebed80ba7bda4d03509d3f46dd977be911389f7b53daefa5ef31d2f7df9402243152e01e83f1b8a9fb300c19d1a0f69a89a9
|
|
Handle this failure in the same way as all other failures: call Invalid()
with the reasons for the failure.
|
|
No callers use the returned value in first_invalid. Remove it from the
function signature and don't set it in the function.
|
|
Split CValidationState into TxValidationState and BlockValidationState
to store validation results for transactions and blocks respectively.
|
