Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
A netmask that contains 1-bits after 0-bits (the 1-bits are not
contiguous on the left side) is invalid [1] [2].
The code before this PR used to parse and accept such
non-left-contiguous netmasks. However, a coming change that will alter
`CNetAddr::ip` to have flexible size would make juggling with such
netmasks more difficult, thus drop support for those.
[1] https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#Subnet_masks
[2] https://tools.ietf.org/html/rfc4632#section-5.1
dac7a111bd refactor: test: use _ variable for unused loop counters (Sebastian Falbesoner)
Pull request description:
This tiny PR substitutes Python loops in the form of `for x in range(N): ...` by `for _ in range(N): ...` where applicable. The idea is indicating to the reader that a block (or statement, in list comprehensions) is just repeated N times, and that the loop counter is not used in the body, hence using the throwaway variable. This is already done quite often in the current tests (see e.g. `$ git grep "for _ in range("`). Another alternative would be using `itertools.repeat` (according to Python core developer Raymond Hettinger it's [even faster](https://twitter.com/raymondh/status/1144527183341375488)), but that doesn't seem to be widespread in use and I'm not sure about a readability increase.
The only drawback I see is that whenever one wants to debug loop iterations, one would need to introduce a loop variable again. Reviewing this is basically a no-brainer, since tests would fail immediately if a a substitution has taken place on a loop where the variable is used.
Instances to replace were found by `$ git grep "for.*in range("` and manually checked.
ACKs for top commit:
darosior:
ACK dac7a111bd
instagibbs:
manual inspection ACK dac7a111bd
practicalswift:
ACK dac7a111bd -- the updated code is easier to reason about since the throwaway nature of a variable is expressed explicitly (using the Pythonic `_` idiom) instead of implicitly. Explicit is better than implicit was we all know by now :)
Tree-SHA512: 5f43ded9ce14e5e00b3876ec445b90acda1842f813149ae7bafa93f3ac3d510bb778e2c701187fd2c73585e6b87797bb2d2987139bd1a9ba7d58775a59392406
4c0731f9c5 Deduplicate missing parents of orphan transactions (Suhas Daftuar)
8196176243 Rewrite parent txid loop of requested transactions (Suhas Daftuar)
Pull request description:
I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter.
This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan.
This addresses a couple of [related](https://github.com/bitcoin/bitcoin/pull/19109#discussion_r455197217) [comments](https://github.com/bitcoin/bitcoin/pull/19109#discussion_r456820373) left in #19109.
ACKs for top commit:
laanwj:
Code review ACK 4c0731f9c5
jonatack:
ACK 4c0731f9c5
ajtowns:
ACK 4c0731f9c5
Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
01cd24c226 doc: set CC_FOR_BUILD when building on OpenBSD (fanquake)
Pull request description:
Closes: #19559
While #19559 has been fixed upstream, it makes sense to not only
recommend using `CC_FOR_BUILD`here until the fix is pulled in as
part of our next libsecp update, but after discussing with Cory,
he suggested we should be setting this on OpenBSD (which still has
the an ancient GCC) regardless.
ACKs for top commit:
real-or-random:
ACK 01cd24c226 I looked at the diff (but can't test the instructions on OpenBSD)
laanwj:
Code review ACK 01cd24c226
Tree-SHA512: 322802b9303771f1be2ad9628f268dfa71dc7ee77948fa2a34f21eceb19b2d8efdd8876c8f0778adbfcde48fa0f88cd4e698ae425428159abca38e8c7980da1d
ca2e474372 Fix a compiler warning: unused GetDevURandom() (Vasil Dimov)
Pull request description:
~~Only define GetDevURandom() if it is going to be used.~~
Silence by planting a dummy reference to the `GetDevURandom` symbol
in the places where we don't call the function.
ACKs for top commit:
practicalswift:
ACK ca2e474372 -- increased signal to noise in compiler diagnostics is good
sipa:
utACK ca2e474372
hebasto:
re-ACK ca2e474372, tested on macOS 10.15.6 + llvm clang 10.0.0
Tree-SHA512: 03c98f00dad5d9a3c5c9f68553d72ad5489ec02f18b9769108a22003ec7be7819a731b1eab6a9f64dafb5be0efddccf6980de7e3bb90cd20d4f4d72f74124675
33a84e8f40 build: Update and sort package list in gitian-linux.yml (Hennadii Stepanov)
95051682be build: Drop old hack which is unneeded now (Hennadii Stepanov)
Pull request description:
The hack was aimed to fix an issue in Ubuntu Trusty 14.04 (see #8188).
The current hack implementation was added in #8315.
On master (8db23349fe) this hack is effectively noop, and it is no longer needed.
I see this PR as a step to removing `libfaketime` from gitian builds.
ACKs for top commit:
dongcarl:
tACK 33a84e8f40
laanwj:
Code review ACK 33a84e8f40
Tree-SHA512: 90036c555a500649ccc3d108bf11f09a9cfd2c92c0b598f7e0c0df63a713ae7abaf78f350b68c025470619c967223f45f6a235ad37a6ce1d1a0341ed34963ba0
9e165d0de4 test: Wait for 'cmpctblock' in p2p_compactblocks when it is expected (Ben Woosley)
Pull request description:
This is a more narrowly-construed wait which eliminates the possibility of the
wait being triggered by other messages.
Note `received_block_announcement` reflect three possible messages:
edec7f7c25/test/functional/p2p_compactblocks.py (L34-L53)
Prompted by looking into: #19449
ACKs for top commit:
laanwj:
Code review ACK 9e165d0de4
theStack:
ACK 9e165d0de4
Tree-SHA512: bc4a9c8bf031c8a7efb40d9625feaa3fd1f56f3b75da7034944af71ccea44328a6c708ab0c13fea85fb7cf4fd9043fe90eb94a25e95b2d42be44c2962b4904ce
fa4dfd215f test: Wait until is_connected in add_p2p_connection (MarcoFalke)
Pull request description:
Moving the wait_until from the individual test scripts to the test framework simplifies two tests
ACKs for top commit:
jnewbery:
Code review ACK fa4dfd215f
theStack:
ACK fa4dfd215f☕
Tree-SHA512: 36eda7eb323614a4c4f9215f1d7b40b9f9c4036d1c08eb701ea705f3e2986fdabd2fc558965a6aadabeed861034aeaeef3c00f968ca17ed7a27e42e506cda87d
566aada386 Test that wtxid relay peers add wtxid to reject filter (Gregory Sanders)
0fea6ede1b Restore test case for p2p transaction blinding (Gregory Sanders)
Pull request description:
Introduced in ca10a03add then erroneously removed in 8d8099e97a. The restored line is how we are
checking that the node will still re-request a specific txid given a witness-related failure.
ACKs for top commit:
fjahr:
tACK 566aada386
Tree-SHA512: be2b75b5eddb88019b79cc798f9922ca7347ccbb2210b8d4eae93fdde62e2cbb614b5247cb2fbd7ee3577dbe053875a9b62c5747aace8617f12790b8fccdeab4
0a8aa626dd refactor: Make HexStr take a span (Wladimir J. van der Laan)
Pull request description:
Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses.
ACKs for top commit:
elichai:
Code review ACK 0a8aa626dd
hebasto:
re-ACK 0a8aa626dd
jonatack:
re-ACK 0a8aa626dd
Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992
1e72b68ab3 Replace `hidden service` with `onion service` (Riccardo Masutti)
Pull request description:
For a couple of years, Tor has made the term `hidden service` obsolete, in favor of `onion service`: [Tor Project | Onion Services](https://community.torproject.org/onion-services/)
This PR updates all the references.
ACKs for top commit:
laanwj:
Code review ACK 1e72b68ab3
hebasto:
ACK 1e72b68ab3, tested on Linux Mint 20 (x86_64).
Tree-SHA512: 6a29e828e1c5e1ec934b5666f67326dbd84d77c8b2641f6740abac6d3d5923b7729763b9ff2230390b0bb23359a5f3731ccd9a30011ca69004f7c820aed17262
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
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
3bd67ba5a4 Test addr response caching (Gleb Naumenko)
cf1569e074 Add addr permission flag enabling non-cached addr sharing (Gleb Naumenko)
acd6135b43 Cache responses to addr requests (Gleb Naumenko)
7cc0e8101f Remove useless 2500 limit on AddrMan queries (Gleb Naumenko)
ded742bc5b Move filtering banned addrs inside GetAddresses() (Gleb Naumenko)
Pull request description:
This is a very simple code change with a big p2p privacy benefit.
It’s currently trivial to scrape any reachable node’s AddrMan (a database of all nodes known to them along with the timestamps).
We do have a limit of one GETADDR per connection, but a spy can disconnect and reconnect even from the same IP, and send GETADDR again and again.
Since we respond with 1,000 random records at most, depending on the AddrMan size it takes probably up to 100 requests for an spy to make sure they scraped (almost) everything.
I even have a script for that. It is totally doable within couple minutes.
Then, with some extra protocol knowledge a spy can infer the direct peers of the victim, and other topological stuff.
I suggest to cache responses to GETADDR on a daily basis, so that an attacker gets at most 1,000 records per day, and can’t track the changes in real time. I will be following up with more improvements to addr relay privacy, but this one alone is a very effective. And simple!
I doubt any of the real software does *reconnect to get new addrs from a given peer*, so we shouldn’t be cutting anyone.
I also believe it doesn’t have any negative implications on the overall topology quality. And the records being “outdated” for at most a day doesn’t break any honest assumptions either.
ACKs for top commit:
jnewbery:
reACK 3bd67ba5a4
promag:
Code review ACK 3bd67ba5a4.
ariard:
Code Review ACK 3bd67ba
Tree-SHA512: dfa5d03205c2424e40a3f8a41af9306227e1ca18beead3b3dda44aa2a082175bb1c6d929dbc7ea8e48e01aed0d50f0d54491caa1147471a2b72a46c3ca06b66f
Introduced in ca10a03add then erroneously removed in
8d8099e97a. The restored line is how we are
checking that the node will still re-request a specific txid given a witness-related failure.
334de75885 scripted-diff: Remove Reference Links (Robert)
Pull request description:
Removed all reference links.
Found this issue from #19582.
The decision to remove links instead of update them was made in #19584
The author of that PR was slow to resolve his commit to use scripted diff so I made this PR instead.
ACKs for top commit:
laanwj:
ACK 334de75885
MarcoFalke:
ACK 334de75885
Tree-SHA512: a337116379912b27974867bd86ec7799a1d41d67b51771885467fbe1be003b415cb37ce8e521568bf3eae190ab2f6af0d6e29fd3ea25f2689b8fb31def8fec96
05e2740d92 doc: Add historical release notes for 0.20.1 (Wladimir J. van der Laan)
Pull request description:
ACKs for top commit:
MarcoFalke:
ACK 05e2740d92
Tree-SHA512: 8f33403c51bcbaf97410642878bbc5ee4cbced2e079a61362a69c7eb9d682cf8d3d6ce16fe9aa99c2c89f21a57910032219312403b16d322d403b029f39c62ff
