a0fc076476 refactor: test/bench: dedup Build{Crediting,Spending}Transaction() (Sebastian Falbesoner)
Pull request description:
prototypes used in `src/test/script_tests.cpp`:
- `CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue = 0);`
- `CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CScriptWitness& scriptWitness, const CTransaction& txCredit);`
prototypes used in `bench/verify_script.cpp`:
- `CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey);`
- `CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CMutableTransaction& txCredit);`
The more generic versions from the script tests are moved into `setup_common.cpp` and the calls are adapted accordingly in the verify_script benchmark (passing the nValue of 1 explicitely for `BuildCreditingTransaction()`, passing empty scriptWitness explicitely and converting txCredit parameter to CTransaction in `BuildSpendingTransaction()`).
Top commit has no ACKs.
Tree-SHA512: 8444f8a18f15070eeec1e5dfd255b55a851dfc2e6647c12b1995a6f7abd7196e830db2181d0e860bcd4cf4c815967584a3756dd450346bca70649dd1d4493e04
prototypes used in src/test/script_tests.cpp:
- CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue = 0);
- CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CScriptWitness& scriptWitness, const CTransaction& txCredit);
prototypes used in bench/verify_script.cpp:
- CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey);
- CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CMutableTransaction& txCredit);
The more generic versions from the script tests are moved into a new file pair
transaction_utils.cpp/h and the calls are adapted accordingly in the
verify_script benchmark (passing the nValue of 1 explicitely for
BuildCreditingTransaction(), passing empty scriptWitness explicitely and
converting txCredit parameter to CTransaction in BuildSpendingTransaction()).
7d8d3e6a2a Add tests for util/vector.h's Cat and Vector (Pieter Wuille)
e65e61c812 Add some general std::vector utility functions (Pieter Wuille)
Pull request description:
This is another general improvement extracted from #16800 .
Two functions are added are:
* Vector(arg1,arg2,arg3,...) constructs a vector with the specified arguments as elements. The vector's type is derived from the arguments. If some of the arguments are rvalue references, they will be moved into place rather than copied (which can't be achieved using list initialization).
* Cat(vector1,vector2) returns a concatenation of the two vectors, efficiently moving elements when relevant.
Vector generalizes (and replaces) the `Singleton` function in src/descriptor.cpp, and `Cat` replaces the function in bech32.cpp
ACKs for top commit:
laanwj:
ACK 7d8d3e6a2a
MarcoFalke:
ACK 7d8d3e6a2a (enjoyed reading the tests, but did not compile)
Tree-SHA512: 92325f14e90d7e7d9d920421979aec22bb0d730e0291362b4326cccc76f9c2d865bec33a797c5c0201773468c3773cb50ce52c8eee4c1ec1a4d10db5cf2b9d2a
f59bbb61af test: Fix bug in blockfilter_index_tests. (Jim Posen)
Pull request description:
The test case tests a chain reorganization, however the two chains were generated in the same manner and thus produced the same blocks.
This issue was [pointed out](https://github.com/bitcoin/bitcoin/pull/14121#discussion_r334282663) by MarcoFalke.
ACKs for top commit:
MarcoFalke:
Thanks! ACK f59bbb61af (looked at the diff on GitHub, didn't compile, nor run tests)
Tree-SHA512: a2f063ae9312051ffc2a3fcc1116a6a8ac09beeef261bc40aa3ff7270ff4de22a790eb19fec6b15ba1eb46e78f1f317bfd91472d8581b95bb9441a56b102554e
084e17cebd Remove unused includes (practicalswift)
Pull request description:
As requested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/16273#issuecomment-521332089:
This PR removes unused includes.
Please note that in contrast to #16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier.
I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup.
Rationale:
* Avoids unnecessary re-compiles in case of header changes.
* Makes reasoning about code dependencies easier.
* Reduces compile-time memory usage.
* Reduces compilation time.
* Warm fuzzy feeling of being lean :-)
ACKs for top commit:
ryanofsky:
Code review ACK 084e17cebd. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway.
Tree-SHA512: 89de56edc6ceea4696e9579bccff10c80080821685b9fb4e8c5ef593b6e43cf662f358788701bb09f84867693f66b2e4db035b92b522a0a775f50b7ecffd6a6d
d7820a1250 util: Filter control characters out of log messages (Wladimir J. van der Laan)
Pull request description:
Belts and suspenders: make sure outgoing log messages don't contain potentially suspicious characters, such as terminal control codes.
This escapes control characters except newline ('\n') in C syntax. It escapes instead of removes them to still allow for troubleshooting issues where they accidentally end up in strings (it is a debug log, after all).
(more checks could be added such as UTF-8 validity and unicode code-point range checking—this is substantially more involved and would need to keep track of state between characters and even `LogPrint` calls as they could end up split up—but escape codes seem to be the most common attack vector for terminals.)
ACKs for top commit:
practicalswift:
ACK d7820a1250 - tested and works as expected :)
Tree-SHA512: 0806265addebdcec1062a6def3e903555e62ba5e93967ce9ee6943d16462a222b3f41135a5bff0a76966ae9e7ed75f211d7785bceda788ae0b0654bf3fd891bf
Belts and suspenders: make sure outgoing log messages don't contain
potentially suspicious characters, such as terminal control codes.
This escapes control characters except newline ('\n') in C syntax.
It escapes instead of removes them to still allow for troubleshooting
issues where they accidentally end up in strings.
bb36372b8f test: add unit tests for Span-parsing helpers (Sebastian Falbesoner)
5e69aeec3f Add documenting comments to spanparsing.h (Pieter Wuille)
230d43fdbc Abstract out some of the descriptor Span-parsing helpers (Pieter Wuille)
Pull request description:
As suggested here: https://github.com/bitcoin/bitcoin/pull/16800#issuecomment-531605482.
This moves the Span parsing functions out of the descriptor module, making them more easily usable for other parsers (in particular, in preparation for miniscript parsing).
ACKs for top commit:
MarcoFalke:
ACK bb36372b8f
Tree-SHA512: b5c5c11a9bc3f0a1c2c4cfa22755654ecfb8d4b69da0dc1fb9f04e1556dc0f6ffd87ad153600963279ac465d587d7971b53d240ced802d12693682411ac73deb
5c2987636f tests: Remove TRANSACTION_DESERIALIZE (replaced by transaction fuzzer) (practicalswift)
0a573682f2 tests: Add fuzzing harness for CheckTransaction(...), IsStandardTx(...) and other CTransaction related functions (practicalswift)
Pull request description:
Add fuzzing harness for `CheckTransaction(...)`, `IsStandardTx(...)` and other `CTransaction` related functions.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/transaction
…
# And to to quickly verify that the relevant code regions are triggered, that the
# fuzzing throughput seems reasonable, etc.
$ contrib/devtools/test_fuzzing_harnesses.sh '^transaction$'
```
`test_fuzzing_harnesses.sh` can be found in PR #17000.
ACKs for top commit:
MarcoFalke:
ACK 5c2987636f
Tree-SHA512: 2f422df795c9dca13c98209ca9ce0fe5a0d4a71fb052fa33d599cc9c9f1d637fee27d58d02ed17b956b3e3d40931cbc1367fc99aa2e882473e54d95dee04d6b7
fa25f43ac5 p2p: Remove BIP61 reject messages (MarcoFalke)
Pull request description:
Reject messages (BIP 61) appear in the following settings:
* Parsing of reject messages (in case `-debug=net` is set, off by default). This has only been used for a single `LogPrint` call for several releases now. Such logging is completely meaningless to us and should thus be removed.
* The sending of reject messages (in case `-enablebip61` is set, off by default). This can be used to debug a node that is under our control. Instead of hacking this debugging into the p2p protocol, it could be more easily achieved by parsing the debug log. (Use `-printtoconsole` to have it as stream, or read from the `debug.log` file like our python function `assert_debug_log` in the test framework does)
Having to maintain all of this logic and code to accommodate debugging, which can be achieved by other means a lot easier, is a burden. It makes review on net processing changes a lot harder, since the reject message logic has to be carried around without introducing any errors or DOS vectors.
ACKs for top commit:
jnewbery:
utACK fa25f43ac5
laanwj:
I'm still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43ac5.
ryanofsky:
Code review ACK fa25f43ac5
Tree-SHA512: daf55254202925e56be3d6cfb3c1c804e7a82cecb1dd1e5bd7b472bae989fd68ac4f21ec53fc46751353056fd645f7f877bebcb0b40920257991423a3d99e0be
4bb660be90 Add release note (Andrew Chow)
ed96b295d7 Update descriptors.md to include sortedmulti (Andrew Chow)
80be78ea75 Test sortedmulti descriptor using BIP 67 tests (Andrew Chow)
6f588fd227 Add sortedmulti descriptor and unit tests (Andrew Chow)
Pull request description:
Adds a `sortedmulti()` descriptor as mentioned in https://github.com/bitcoin/bitcoin/pull/17023#issuecomment-537596416.
`sortedmulti()` works in the same way as `multi` does but sorts the pubkeys in the resulting scripts in lexicographic order as described in [BIP67](https://github.com/bitcoin/bips/blob/master/bip-0067.mediawiki). Note that this does not add support for BIP67 nor is BIP67 fully supported by this descriptor (which is why it is not named `multi67()`) as it does not require compressed pubkeys.
Tests from BIP67 were added and documentation was updated.
ACKs for top commit:
instagibbs:
re-ACK 4bb660be90
Sjors:
re-ACK 4bb660be90
Tree-SHA512: 93b21112a74ebe0bf316d8f3e0291f69fd975cf0a29332f9728e7b880cad312b8b14007e86adcd7899f117b9303cbcf4cb35f3bb2f2f648d1a446f83f75a70a5
43e7d576f5 doc: Improve test READMEs (Fabian Jahr)
Pull request description:
General improvements on READMEs for unit tests and functional tests:
- Give unit test readme a headline
- Move general information on `src/test` folder to the top
- Add information on logging and debugging unit tests
- Improve debugging and logging information in functional testing
- Include all available log levels in functional tests
ACKs for top commit:
laanwj:
ACK 43e7d576f5
Tree-SHA512: 22b27644992ba5d99a885cd51b7a474806714396fcea1fd2d6285e41bdf3b28835ad8c81449099e3ee15a63d57b3ab9acb89c425d9855ed1d9b4af21db35ab03
efd2474d17 util: CBufferedFile fixes (Larry Ruane)
Pull request description:
The `CBufferedFile` object guarantees its user is able to "rewind" the data stream (that's being read from a file) up to a certain number of bytes, as specified by the user in the constructor. This guarantee is not honored due to a bug in the `SetPos` method.
Such rewinding is done in `LoadExternalBlockFile()` (currently the only user of this object), which deserializes a series of `CBlock` objects. If that function encounters something unexpected in the data stream, which is coming from a `blocks/blk00???.dat` file, it "rewinds" to an earlier position in the stream to try to get in sync again. The `CBufferedFile` object does not actually rewind its file offset; it simply repositions its internal offset, `nReadPos`, to an earlier position within the object's private buffer; this is why there's a limit to how far the user may rewind.
If `LoadExternalBlockFile()` needs to rewind (call `blkdat.SetPos()`), the stream may not be positioned as it should be, causing errors in deserialization. This need to rewind is probably rare, which is likely why this bug hasn't been noticed already. But if this object is used elsewhere in the future, this could be a serious problem, especially as, due to the nature of the bug, the `SetPos()` _sometimes_ works.
This PR adds a unit test for `CBufferedFile` that fails due to this bug. (Until now it has had no unit tests.) The unit test provides good documentation and examples for developers trying to understand `LoadExternalBlockFile()` and for future users of this object.
This PR also adds code to throw an exception from the constructor if the rewind argument is not less than the buffer size (since that doesn't make any sense).
Finally, I discovered that the object is too restrictive in one respect: When the deserialization methods call this object's `read` method, a check ensures that the number of bytes being requested is less than the size of the buffer (adjusting for the rewind size), else it throws an exception. This restriction is unnecessary; the object being deserialized can be larger than the buffer because multiple reads from disk can satisfy the request.
ACKs for top commit:
laanwj:
ACK ~after squash.~ efd2474d17
mzumsande:
I had intended to follow up earlier on my last comment, ACK efd2474d17. I reviewed the code, ran tests and did a successful reindex on testnet with this branch.
Tree-SHA512: 695529e0af38bae2af4e0cc2895dda56a71b9059c3de04d32e09c0165a50f6aacee499f2042156ab5eaa6f0349bab6bcca4ef9f6f9ded4e60d4483beab7e4554
0c62e3aa73 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
38bfca6bb2 Added comments referencing multiple CVEs in tests and production code. (lucash-dev)
Pull request description:
This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
This improves developer experience by making understanding the tests easier.
ACKs for top commit:
laanwj:
ACK 0c62e3aa73, checked the CVE numbers, thanks for adding documentation
Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9
8573429d46 test: add some unit tests for merkle.cpp (soroosh-sdi)
Pull request description:
following situations are covered:
- empty block
- one Tx
- Merkle root of a block with odd Txs should not change with repeating
last one
- Merkle root is computed with combining Merkle root of left subtree and right subtree
- block witness is Merkle root of a block when setting first Tx
to zero.
Signed-off-by: soroosh-sdi <soroosh.sardari@gmail.com>
ACKs for top commit:
laanwj:
ACK 8573429d46
Tree-SHA512: e12228171de8f8480f173c9d9d0359f00f46bf09075e0767f5f1a367478a1b7b6d177d230f7e930914915cd2c6b66b18d24b1682f1233c38e97954ba331e5773
following situations are covered:
- empty block
- one Tx
- Merkle root of a block with odd Txs should not change with repeating
last one
- Merkle root is computed with combining Merkle root of left subtree and right subtree
- block witness is Merkle root of a block when setting first Tx
to zero.
Signed-off-by: soroosh-sdi <soroosh.sardari@gmail.com>
3bf9d8cac0 Testchains: Qt: Simplify network/chain styles (Jorge Timón)
052c54ecb0 Testchains: Generic selection with -chain=<str> in addition of -testnet and -regtest (Jorge Timón)
Pull request description:
Separated from #8994 as suggested by MarcoFalke and Sjors in https://github.com/bitcoin/bitcoin/pull/8994#issuecomment-522555390
You can't really test the qt changes on their own, so to test them, use #8994 .
ACKs for top commit:
MarcoFalke:
ACK 3bf9d8cac0
Tree-SHA512: 5b5e6083ebc0a44505a507fac633e7af18037c85e5e73f5d1e6f7e730575d3297ba8a31d1c2441df623b273f061c32d8fa324f4aa6bead01d23e88582029b568
0ba08020c9 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba91e1 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489027 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885306 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a4bb Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39cd42 Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba822 [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dbac9b [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bce29 Remove unused variable (Suhas Daftuar)
Pull request description:
Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.
(b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)
ACKs for top commit:
sipa:
ACK 0ba08020c9
ajtowns:
ACK 0ba08020c9 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
TheBlueMatt:
re-utACK 0ba08020c9. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
jnewbery:
utACK 0ba08020c9
jamesob:
ACK 0ba08020c9
Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf
Transaction relay is primarily optimized for balancing redundancy/robustness
with bandwidth minimization -- as a result transaction relay leaks information
that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of
a given transaction.
(b) Knowledge of the network graph could be used to split a target node or
nodes from the honest network (eg by knowing which peers to attack in order to
achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction
relay; inferring network connectivity from the relay of blocks/block headers is
much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that
are only used for block relay. (In the future, we might consider rotating our
transaction-relay peers to help limit the effects of (a).)
9b92538ade Remove unused fScriptChecks parameter from CheckInputs (Matt Corallo)
Pull request description:
fScriptChecks = false just short-circuits the entire function, so
passing it in is entirely useless.
This is extracted from #13233 /cc TheBlueMatt.
Recommend reviewing with `git show --ignore-all-space`, i.e.:
https://github.com/bitcoin/bitcoin/pull/13868/files?w=1
ACKs for top commit:
TheBlueMatt:
utACK 9b92538ade. Checked diff had no functional change and new comment copy looks correct.
kallewoof:
ACK 9b92538ade
ajtowns:
ACK 9b92538ade ; code review, checked tests work. Looks right to me, and fanquake's notes make sense. Could change the coinbase early exit to `assert(!tx.IsCoinBase());`.
fanquake:
ACK 9b92538ade - Notes / testing below.
Tree-SHA512: add253a3e8cf4b33eddbc49efcec333c14b5ea61c7d34e43230351d40cff6adc919a75b91c72c4de8647a395284db74a61639f4c67848d4b2fec3a705b557790
b9ee63c71b Make descriptor test deterministic (David Reikher)
Pull request description:
This is an improvement to a test, inspired by #14343 - removing non determinism from a test.
The test `descriptor_test` is non-deterministic, as it relies on the `MaybeUseHInsteadOfApostrophy` function which randomly either swaps all apostrophes with 'h' or doesn't at all in a descriptor. This fix makes both cases always run, if an apostrophe is found in a test descriptor.
This does not reduce test coverage but removes the non-determinism.
Additionally, the `MaybeUseHInsteadOfApostrophy` function removed the checksum if found at the end of a descriptor when the apostrophes are swapped by 'h's, since after being swapped the checksum is no longer correct. I instead added re-calculation of the checksum using the `DescriptorChecksum` function, which adds coverage for the case of a descriptors having 'h's instead of apostrophes and a checksum. This was previously lacking.
To achieve this I had to move `DescriptorChecksum` and `PolyMod` out of the anonymous namespace in descriptor.cpp to make `DescriptorChecksum` accessible in descriptor_tests.cpp.
All tests complete successfully (functional as well as unit tests).
ACKs for top commit:
achow101:
Code Review ACK b9ee63c71b
Tree-SHA512: 992c73a6644a07bfe7c72301ee2666f3c4845a012aaedd7a099a05cea8bdac84fa8280b28e44a7856260c00c0be1a6f1b6768f5694c2a22edf4c489e53fec424
Changed MaybeUseHInsteadOfApostrophy to UseHInsteadOfApostrophe.
This function now always replaces apostrophes with 'h'.
The original Check function was renamed to DoCheck and it's
called with an additional parameter which tells it to either
leave the prv and pub arguments as is or replace the apostrophes
with 'h'. The test runs with apostrophes replaced in prv only,
pub only, prv and pub and without replacement at all. Replacement
of apostrophes in a descriptor and then running DoCheck is conditional
on whether apostrophes are found in that descriptor.
Additionally, instead of dropping the checksum recalculate it
after replacing apostrophes with 'h' in the function UseHInsteadOfApostrophe
using the GetDescriptorChecksum function. That way, this also
introduces an indirect unit test to GetDescriptoChecksum.
