90bd476ea6 build: make clean removes .gcda and .gcno files from fuzz directory (eugene)
Pull request description:
I believe these should also be deleted upon invoking `make clean`. It also garbles the coverage file if you try to fuzz the same harness again.
ACKs for top commit:
practicalswift:
ACK 90bd476ea6 -- patch looks correct
hebasto:
ACK 90bd476ea6, tested with hints from #12602 and #18107.
darosior:
ACK 90bd476ea6
Tree-SHA512: 4b2eb664f64d18bc0385c5a0040b0b9fa6fe470c941ae39c7cb4544c4283427a8d4985517475fe0295c3ab2794b9a2ad4f76b6a443c05d846c97c966add87ca9
Extract logic that check multiple connection types into interface functions &
structure as switch statements. This makes it very clear what touch points are
for accessing `m_conn_type` & using the switch statements enables the compiler
to warn if a new connection type is introduced but not handled for these cases.
Make the connection counts explicit and extract into interface functions around
m_conn_type. Using explicit counting and switch statements where possible
should help prevent counting bugs in the future.
The desired logic is for us to only open feeler connections after we have hit
the max count for outbound full relay connections. A short lived AddrFetch
connection (previously called oneshot) could cause ThreadOpenConnections to
miscount and mistakenly open a feeler instead of full relay.
For a couple of years, Tor documentation has made
the term hidden service obsolete, in favor of onion
service.
This PR updates all the references in the code base.
edc316020e test: Remove duplicate NodeContext hacks (Russell Yanofsky)
Pull request description:
Qt tests currently are currently using two NodeContext structs at the same time, one in interfaces::NodeImpl::m_context, and the other in BasicTestingSetup::m_node, and the tests have hacks transferring state between them.
Fix this by getting rid of the NodeImpl::m_context struct and making it a pointer. This way a common BitcoinApplication object can be used for all qt tests, but they can still have their own testing setups.
Non-test code is changing but non-test behavior is still the same as before.
Motivation for this PR is to be able to remove the "std::move(test.m_node.connman)" and mempool hacks for swapping individual NodeContext members in Qt tests, because followup PR #19099 adds yet another member (wallet_client) that needs to be swapped. After this change, the whole NodeContext struct can be swapped instead of individual members, so the workarounds are less fragile and invasive.
ACKs for top commit:
MarcoFalke:
crACK edc316020e🌮
promag:
ACK edc316020e.
Tree-SHA512: c1650e4127f43a4020304ca7c13b5d9122fb5723aacd8fa1cf855d03c6052fcfb7685810aa2a5ef708561015f0022fecaacbad479295104ca45d2c17579466a4
9f88ded82b test addition of unknown segwit spends to txid reject filter (Gregory Sanders)
7989901c7e Add txids with non-standard inputs to reject filter (Suhas Daftuar)
Pull request description:
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
ACKs for top commit:
ajtowns:
ACK 9f88ded82b - code review
jnewbery:
Code review ACK 9f88ded82b
ariard:
Code Review/Tested ACK 9f88ded
naumenkogs:
utACK 9f88ded82b
jonatack:
ACK 9f88ded82b
Tree-SHA512: 1e93c0a5b68cb432524780ffc0093db893911fdfed9e2ed17f888e59114cc75d2a07062aefad4e5ce2e87c9270886117a8abb3c78fb889c9b9f31967f1777148
substitutes "for x in range(N):" by "for _ in range(N):"
indicates to the reader that a block is just repeated N times, and
that the loop counter is not used in the body
70452a070b build: set minimum required Boost to 1.58 (fanquake)
Pull request description:
Any systems which only have an older installable Boost can use depends.
1.58.0 retains compatibility with the packages [installable on Ubuntu 16.04](https://packages.ubuntu.com/xenial/libboost-dev).
The projects usage of Boost wont be going away any time soon, if ever (i.e #15382), and our usage of the test framework.
Fixes: #19506
ACKs for top commit:
practicalswift:
ACK 70452a070b -- patch looks correct
laanwj:
ACK 70452a070b
hebasto:
ACK 70452a070b, tested on Linux Mint 20 (x86_64).
Tree-SHA512: d290415e3c70a394b3d7659c0480a35b4082bdce8d48b1c64a0025f7ad6e21567b4dc85813869513ad246d27f950706930410587c11c1aa3693ae6245084765c
With this commit, make clean now removes coverage files from the
fuzzing directory. Without this, subsequent fuzzing runs would have
garbled coverage signals for files in the fuzz directory as
they were never deleted with make clean.
fa5288cf5c contrib: Fixup valgrind suppressions file (MarcoFalke)
Pull request description:
I am observing this one on bionic with system boost::fs:
```
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:__wcsnlen_avx2
fun:wcsnrtombs
fun:_ZNKSt7codecvtIwc11__mbstate_tE6do_outERS0_PKwS4_RS4_PcS6_RS6_
fun:_ZN5boost10filesystem11path_traits7convertEPKwS3_RNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt7codecvtIwc11__mbstate_tE
fun:_ZN5boost10filesystem6detail11unique_pathERKNS0_4pathEPNS_6system10error_codeE
...
ACKs for top commit:
practicalswift:
ACK fa5288cf5c -- patch looks correct
Tree-SHA512: 067e10e932a7f5b13e516134e0cfd3030265c1b582cdfde1cea97042e31399aa40c4590710a39429854c68ad703a0ae9f0b06e9af4cdd81e3cacb042939a84b6
1d8338d6b7 util: use HAVE_FDATASYNC to determine fdatasync() use (fanquake)
Pull request description:
Rather than just using on Linux and NetBSD, use `fdatasync()` based
on whether it's available. i.e `fdatasync` is available in newer versions of FreeBSD.
This also aligns more closely with what is being done in leveldb.
Was pointed out by Luke in #19430.
ACKs for top commit:
practicalswift:
ACK 1d8338d6b7 -- patch looks correct
laanwj:
ACK 1d8338d6b7
hebasto:
ACK 1d8338d6b7
Tree-SHA512: 7dd6d87f5dc0c0ba21ae42f96b63fc12b34806cd536457fc4284f14bb8c235765344be228b000c6adf4cd1e8c4e6a03a18ca18ab22599c42cc3b706e0bcd1a17
82fc4017b7 test: Catch decimal.InvalidOperation from TestNodeCLI#send_cli (Ben Woosley)
Pull request description:
`decimal.InvalidOperation` is a special case of a float parsing error, which
presumably should be handled in the same way as a general parsing error,
rather than blow up.
Alternatives include: logging the error, or re-raising with more information.
Example log output:
```
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 603, in sync_all
self.sync_blocks(nodes)
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 568, in sync_blocks
best_hash = [x.getbestblockhash() for x in rpc_connections]
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 568, in <listcomp>
best_hash = [x.getbestblockhash() for x in rpc_connections]
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_node.py", line 571, in __call__
return self.cli.send_cli(self.command, *args, **kwargs)
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_node.py", line 639, in send_cli
return json.loads(cli_stdout, parse_float=decimal.Decimal)
File "/usr/lib64/python3.6/json/__init__.py", line 367, in loads
return cls(**kw).decode(s)
File "/usr/lib64/python3.6/json/decoder.py", line 339, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/lib64/python3.6/json/decoder.py", line 355, in raw_decode
obj, end = self.scan_once(s, idx)
decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]
```
See: https://travis-ci.org/github/bitcoin/bitcoin/jobs/713502326
ACKs for top commit:
laanwj:
ACK 82fc4017b7
Tree-SHA512: 8c102b8bf831b05c5ca4b2e1feb5574dcbaed8cab0b2f22b013c5dfcb81788a38839a163dd1e2c6470ccbe5874214663b84485f45467738fd850ca38d539ae25
31cf68a3ad [util] add RunCommandParseJSON (Sjors Provoost)
c17f54ee53 [ci] use boost::process (Sjors Provoost)
32128ba682 [doc] include Doxygen comments for HAVE_BOOST_PROCESS (Sjors Provoost)
3c84d85f7d [build] msvc: add boost::process (Sjors Provoost)
c47e4bbf0b [build] make boost-process opt-in (Sjors Provoost)
929cda5470 configure: add ax_boost_process (Sjors Provoost)
8314c23d7b [depends] boost: patch unused variable in boost_process (Sjors Provoost)
Pull request description:
Prerequisite for external signer support in #16546. Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).
This adds a new dependency [boost process](https://github.com/boostorg/process/tree/boost-1.64.0). This is part of Boost since 1.64 which is part of `depends`. Because the minimum Boost version is 1.47, this functionality is skipped for older versions of Boost.
Use `./configure --with-boost-process` to opt in, which checks for the presence of Boost::Process.
We add `UniValue runCommandParseJSON(const std::string& strCommand)` to `system.{h,cpp}` which calls an arbitrary command and processes the JSON returned by it. This is currently only called by the test suite.
~For testing purposes this adds a new regtest-only RPC method `runcommand`, as well as `test/mocks/command.py` used by functional tests.~ (this is no longer the case)
TODO:
- [ ] review boost process in #15440
ACKs for top commit:
achow101:
ACK 31cf68a3ad
hebasto:
re-ACK 31cf68a3ad, only rebased (verified with `git range-diff`) and removed an unintentional tab character since the [previous](https://github.com/bitcoin/bitcoin/pull/15382#pullrequestreview-458371035) review.
meshcollider:
Very light utACK 31cf68a3ad, although I am not very confident with build stuff.
promag:
Code review ACK 31cf68a3ad, don't mind the nit.
ryanofsky:
Code review ACK 31cf68a3ad. I left some comments below that could be ignored or followed up later. The current change is clean and comprehensive.
Tree-SHA512: c506e747014b263606e1f538ed4624a8ad7bcf4e025cb700c12cc5739964e254dc04a2bbb848996b170e2ccec3fbfa4fe9e2b3976b191222cfb82fc3e6ab182d
72351784b3 lint: Remove travis env var from commit linter (Fabian Jahr)
Pull request description:
#19439 was recently merged and seemed to work fine but I now noticed strange behavior when it was running in Travis, which I could not reproduce locally. It turns out `TRAVIS_COMMIT_RANGE` which is used in Travis to get the commits for the linter, uses all the commits that were in a push, which includes all rebase commits for example. This means that the linter can fail on a commit that the developer has never even seen before, which can be very confusing. See an example here which caused me to look into this: https://travis-ci.org/github/bitcoin/bitcoin/jobs/714296381 The commit that is reported as failing in my PR is not part of my PR.
I think we rather want to use something like `git merge-base` to get the commit range by default and in Travis. I am leaving the env variable functionality in place with a different name but this is not a variable that can be expected to be present in the CI environments so the `merge-base` range should be used there by default.
ACKs for top commit:
hebasto:
ACK 72351784b3, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: afb27bb386855cb8d5cf84fd3a6c11ef1160b25af6175ed0aa146bf04b9a26eb77298df70df0a855f8c46f19f08b3f62c49872c12974fcfa5526a15ee05b3c10
In the logic for requesting missing parents of orphan transactions, parent
transactions with multiple outputs being spent by the given orphan were being
processed multiple times. Fix this by deduplicating the set of missing parent
txids first.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Previously, we would potentially add the same txid many times to the rolling
bloom filter of recently announced transactions to a peer, if many outputs of
the same txid appeared as inputs in a transaction. Eliminate this problem and
avoid redundant lookups by asking the mempool for the unique parents of a
requested transaction.
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
63e9e40b73 test: Add LockStackEmpty() (Hennadii Stepanov)
42b2a95373 test: Repeat deadlock tests (Hennadii Stepanov)
1f96be25b0 Preserve initial state if push_lock() throws exception (Hennadii Stepanov)
Pull request description:
On master (e3fa3c7d67) if the `push_lock()` throws the "potential deadlock detected" exception (via the `potential_deadlock_detected()` call), the `LockData` instance internal state differs from one when the `push_lock()` was called. This non-well behaviour makes (at least) testing brittle.
This PR preserves the `LockData` instance initial state if `push_lock()` throws an exception, and improves the `sync_tests` unit test.
ACKs for top commit:
MarcoFalke:
re-ACK 63e9e40b73
vasild:
ACK 63e9e40
Tree-SHA512: 7679182154ce5f079b44b790faf76eb5f553328dea70a326ff6b600db70e2f9ae015a33a104ca070cb660318280cb79b6b42e37ea5166f26f9e627ba721fcdec
faa9a74c9e test: Fail wait_until early if connection is lost (MarcoFalke)
Pull request description:
Calling `minonode.wait_until` needs a connection to make progress (e.g. waiting for an inv), unless the mininode waits for the initial connection or for a disconnection. So for test development and failure debugging, fail early in all `wait_until`, unless opted out.
ACKs for top commit:
jnewbery:
Code review ACK faa9a74c9e.
Tree-SHA512: 4be850b96e23b87bc2ff42c028a5045d6f5cdbc9482ce6a6ba01cc5eb26710dab9e2ed547c363aac4bd5825151ee9996fb797261420b631bceeddbfa698d1dec
f916847d2b rpc: Document getwalletinfo's unlocked_until field as optional (Justin Moon)
Pull request description:
The `getwalletinfo` RPC command's `unlocked_until` field is [optional in the code](f916847d2b/src/wallet/rpcwallet.cpp (L2397)), but wasn't marked as optional in the docs.
ACKs for top commit:
theStack:
ACK f916847d2b
achow101:
ACK f916847d2b
kristapsk:
ACK f916847d2b
Tree-SHA512: 8d82f0992fdaf8160000acf4a6e7e7f9ff289a90a983be2e078cf754f4b03601637e5f405afa66bd55adef9b347fa5eac5cc1822033b2ac08c587609cf3dfe0f
77c507358b Make Hash[160] consume range-like objects (Pieter Wuille)
02c4cc5c5d Make CHash256/CHash160 output to Span (Pieter Wuille)
0ef97b1b10 Make MurmurHash3 consume Spans (Pieter Wuille)
e549bf8a9a Make CHash256 and CHash160 consume Spans (Pieter Wuille)
2a2182c387 Make script/standard's BaseHash Span-convertible (Pieter Wuille)
e63dcc3a67 Add MakeUCharSpan, to help constructing Span<[const] unsigned char> (Pieter Wuille)
567825049f Make uint256 Span-convertible by adding ::data() (Pieter Wuille)
131a2f0337 scripted-diff: rename base_blob::data to m_data (Pieter Wuille)
Pull request description:
This makes use of the implicit constructions and conversions to Span introduced in #18468 to simplify the hash.h interface:
* All functions that take a pointer and a length are changed to take a Span instead.
* The Hash() and Hash160() functions are changed to take in "range" objects instead of begin/end iterators.
ACKs for top commit:
laanwj:
re-ACK 77c507358b
jonatack:
Code review re-ACK 77c5073 per `git range-diff 14ceddd 49fc016 77c5073`
Tree-SHA512: 9ec929891b1ddcf30eb14b946ee1bf142eca1442b9de0067ad6a3c181e0c7ea0c99c0e291e7f6e7a18bd7bdf78fe94ee3d5de66e167401674caf91e026269771
