873beca6d [tests] Rename NodeConn and NodeConnCB (John Newbery)
Pull request description:
Final step in #11518
NodeConn -> P2PConnection
NodeConnCB -> P2PInterface
This is basically just a rename. Should be an easy review.
Tree-SHA512: fe1204b2b3d8182c5e324ffa7cb4099a47ef8536380e0bb9d37a5fccf76a24f548d1f1a7988ab8f830986a3058b670696de3fc891af5e5f75dbeb4e3273005d7
46ce223d1 Add tests for CMerkleBlock usage with txids specified (James O'Beirne)
5ab586f90 Consolidate CMerkleBlock constructor into a single method (James O'Beirne)
Pull request description:
What started as a simple task to add test coverage ended up giving way to a light refactoring. This consolidates the mostly-identical `CMerkleBlock` constructors into one (using C++11 constructor delegation) and adds coverage for the by-txids construction case.
### Before
### After
Tree-SHA512: eed84ed3e8bfc43473077b575c8252759a857e37275e4b36ca7cc2c17a65895e5f494bfd9d4aeab09fc6e98fc6a9c641ac7ecc0ddbeefe01a9e4308e7909e529
fa013664ae23d0682a195b9bded85bc19c99536e util: Add type safe GetTime (MarcoFalke)
Pull request description:
There are basically two ways to get the time in Bitcoin Core:
* get the system time (via `GetSystemTimeInSeconds` or `GetTime{Millis,Micros}`)
* get the mockable time (via `GetTime`)
Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc38e.
Fix that by deprecating `GetTime` and adding a `GetTime<>` that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible.
ACKs for commit fa0136:
promag:
utACK fa013664.
Tree-SHA512: efab9c463f079fd8fd3030c479637c7b1e8be567a881234bd0f555c8f87e518e3b43ef2466128103db8fc40295aaf24e87ad76d91f338c631246fc703477e95c
308b76732f Fix bug around transaction requests (Suhas Daftuar)
f635a3ba11 Expire old entries from the in-flight tx map (Suhas Daftuar)
e32e08407e Remove NOTFOUND transactions from in-flight data structures (Suhas Daftuar)
23163b7593 Add an explicit memory bound to m_tx_process_time (Suhas Daftuar)
218697b645 Improve NOTFOUND comment (Suhas Daftuar)
Pull request description:
#14897 introduced several bugs that could lead to a node no longer requesting transactions from one or more of its peers. Credit to ajtowns for originally reporting many of these bugs along with an originally proposed fix in #15776.
This PR does a few things:
- Fix a bug in NOTFOUND processing, where the in-flight map for a peer was keeping transactions it shouldn't
- Eliminate the possibility of a memory attack on the CNodeState `m_tx_process_time` data structure by explicitly bounding its size
- Remove entries from a peer's in-flight map after 10 minutes, so that we should always eventually resume transaction requests even if there are other bugs like the NOTFOUND one
- Fix a bug relating to the coordination of request times when multiple peers announce the same transaction
The expiry mechanism added here is something we'll likely want to remove in the future, but is belt-and-suspenders for now to try to ensure we don't have other bugs that could lead to transaction relay failing due to some unforeseen conditions.
ACKs for commit 308b76:
ajtowns:
utACK 308b76732f97020c86977e29c854e8e27262cf7c
morcos:
light ACK 308b767
laanwj:
Code review ACK 308b76732f97020c86977e29c854e8e27262cf7c
jonatack:
Light ACK 308b76732f97020c86977e29c854e8e27262cf7c.
jamesob:
ACK 308b76732f
MarcoFalke:
ACK 308b76732f97020c86977e29c854e8e27262cf7c (Tested two of the three bugs this pull fixes, see comment above)
jamesob:
Concept ACK 308b76732f
MarcoFalke:
ACK 308b76732f
Tree-SHA512: 8865dca5294447859d95655e8699085643db60c22f0719e76e961651a1398251bc932494b68932e33f68d4f6084579ab3bed7d0e7dd4ac6c362590eaf9414eda
1cff3d6cb0 Change in transaction pull scheduling to prevent InvBlock-related attacks (Gleb Naumenko)
Pull request description:
This code makes executing two particular (and potentially other) attacks harder.
### InvBlock
This behavior was described well [here](https://www.cs.umd.edu/projects/coinscope/coinscope.pdf) (page 11).
Per current implementation, if node A receives _INV_ (tx) from node B, node A sends _GETDATA_ to B and waits for _TX_ message back.
Node A is likely to receive more _INVs_ (regarding the same tx) from other peers. But node A would not send another _GETDATA_ unless it does not hear _TX_ back from node B for next 2 minutes (to save bandwidth)
Thus, if B is a malicious node, it can prevent node A from getting the transaction (even if all A’s peers have it) for 2 minutes.
This behavior seems to be an inherent limitation of the current P2P relay protocol, and I don’t see how it can be fundamentally changed (I can see workarounds which involve rewriting a lot of P2P code though).
### What does this PR fix?
The attacks I’m looking at involve preventing A from learning the transaction for 2*N minutes. To do that, an attacker has to spin up N nodes and send N _INVs_ simultaneously to node A (then InvBlocks will be queued with an interval of 2 minutes according to current implementation)
More precisely, 2 scenarios I’m looking at are:
1. An attacker censors a particular transaction. By performing InvBlock from different nodes, an attacker can execute a network-wide censorship of a particular transaction (or all transactions). The earlier an attacker founds the transaction he wants to censor, the easier it is to perform an attack. As it was pointed out by @gwillen, this is even more dangerous in the case of lightning, where transactions are known in advance.
2. Topology inference described in papers [1](https://www.cs.umd.edu/projects/coinscope/coinscope.pdf), [2](https://arxiv.org/pdf/1812.00942.pdf) involve network-wide InvBlock. This fix would not mitigate this type of inference, but I believe it will make it more expensive to perform (an attacker would have to create more transactions and perform more rounds to learn the topology, the second paper itself notes that InvBlock isolation is important for the attack).
### How does it work
This PR introduces bias toward outbound connections (they have higher priority when a node chooses from whom it should request a transaction) and randomizes the order.
As per @gmaxwell suggestion, GETDATA requests queue is created after processing all incoming messages from all nodes.
After this fix, if the incoming messages were [I1, I2, I3, O1, O2, O3, O4], the queue for _GETDATA_ may look like [O2, O1, O3, O4, I1, I3, I2, ….].
If {I1, I2, I3} were significantly earlier (but the difference is less than TX_TIMEOUT=60 s) than others, the queue for _GETDATA_ may look like [I2, O2, O1, O3, O4, I1, I3, ….].
### Other comments:
1. This mitigation works better if the connectivity is higher (especially outbound, because it would be less likely that 2 _GETDATAs_ for inbound malicious nodes queued together)
Tree-SHA512: 2ad1e80c3c7e16ff0f2d1160aa7d9a5eaae88baa88467f156b987fe2a387f767a41e11507d7f99ea02ab75e89ab93b6a278d138cb1054f1aaa2df336e9b2ca6a
ee5efad6cf [tests] refactor node_network_limited (John Newbery)
b425131f5a [tests] remove redundant duplicate tests from node_network_limited (John Newbery)
2e02984591 [tests] node_network_limited - remove race condition (John Newbery)
dbfe294805 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery)
1285312048 [tests] fix flake8 warnings in node_network_limited.py (John Newbery)
Pull request description:
Fixes race condition in the node_network_limited test case introduced in #11740. Also tidies up the test and removes redundant duplicate tests.
Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)
Pull request description:
## Problem
`BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
thrown, but not which one.
Here's an example of how this can cause a test to pass when a developer
introduces a consensus bug. The test for the sigops limit assumes
that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
also fail with bad-txns-vout-negative, if a naive developer lowers
`BLOCKSUBSIDY` to `1*COIN`.
## Solution
`BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
commit uses this for all exceptions that are checked for in
`miner_tets.cpp`:
* `bad-blk-sigops`
* `bad-cb-multiple`
* `bad-txns-inputs-missingorspent`
* `block-validation-failed`
If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:
<img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">
## Other considerations
A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.
I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.
Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.
Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
01013f5 Simplify tx validation tests (Pieter Wuille)
2dd6f80 Add a test that all flags are softforks (Pieter Wuille)
2851b77 Make all script verification flags softforks (Pieter Wuille)
Pull request description:
This change makes `SCRIPT_VERIFY_UPGRADABLE_NOPS` not apply to `OP_CHECKLOCKTIMEVERIFY` and `OP_CHECKSEQUENCEVERIFY`. This is a no-op as `UPGRADABLE_NOPS` is only set for mempool transactions, and those always have `SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY` and `SCRIPT_VERIFY_CHECKSEQUENCEVERIFY` set as well. The advantage is that setting more flags now always results in a reduction in acceptable scripts (=softfork).
This results in a nice and testable property for validation, for which a new test is added.
This also means that the introduction of a new definition for a NOP or witness version will likely need the following procedure (example OP_NOP8 here)
* Remove OP_NOP8 from being affected by `SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS`.
* Add a `SCRIPT_VERIFY_DISCOURAGE_NOP8`, which only applies to `OP_NOP8`.
* Add a `SCRIPT_VERIFY_NOP8` which implements the new consensus logic.
* Before activation, add `SCRIPT_VERIFY_DISCOURAGE_NOP8` to the mempool flags.
* After activation, add `SCRIPT_VERIFY_NOP8` to both the mempool and consensus flags.
Tree-SHA512: d3b4538986ecf646aac9dba13a8d89318baf9e308e258547ca3b99e7c0509747f323edac6b1fea4e87e7d3c01b71193794b41679ae4f86f6e11ed6be3fd62c72
6558f8acc [gui] Defer coin control instancing (João Barbosa)
Pull request description:
Defer the GUI coin control instancing so that argument processing
is taken into account for the default coin control values.
Fixes#12312
Tree-SHA512: ecda28b94f4709319e9484b01afe763c7c3569097d2afb89db79da8a195c46d20ea77166df7edce0c8ab77627b295def01c072148714503436d27675d5e75d99
b21244e0be Updating benchmarkmarking.md with an updated sample output and help options (Jeff Rade)
Pull request description:
This PR is just a documentation update for someone (or myself) that looks into finishing up #7883 in the future.
Looked through #7883 and appears [ryanofsky's PR](https://github.com/bitcoin/bitcoin/pull/8873) setup the benchmarks, but there are `FIXME` comments to pull in data from `test/` to get a larger data set (assuming reason why 7883 is still open).
Tree-SHA512: d758efc659c75f2b3ceb376f5a466c4234354077e4671ac3eb901c082c4e519ce5ff592cea4742711050b4ce56a1b65ef69433dd74e7db3eb11a8567d517d9e2
* [tests] Remove mininode periodic (half-hour) ping messages
* [tests] Tidy up mininode
Add docstrings and renames some methods.
Also removes the redundant NodeConn.readable() method override.
* [tests] Move only: move NodeConnCB below NodeConn
This is required since NodeConnCB will inherit from NodeConn
after the next commit.
* [tests] Make NodeConnCB a subclass of NodeConn
This makes NodeConnCB a subclass of NodeConn, and
removes the need for the client code to know
anything about the implementation details of NodeConnCB.
NodeConn can now be swapped out for any other implementation
of a low-level connection without changing client code.
* [tests] Move version message sending from NodeConn to NodeConnCB
This commit moves the logic that sends a version message
on connection from NodeConn to NodeConnCB. NodeConn should
not be aware of the semantics or meaning of the P2P payloads.
* remove witness
Signed-off-by: Pasta <pasta@dashboost.org>
* Fix 11712
Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
