| Age | Commit message (Collapse) | Author |
|---|
|
fa77de2baa40ee828c850ef4068c76cc3619e87b rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke)
fa50bdc755489b2e291ea5ba0e39e44a20c6c6de rpc: Limit echo to 10 args (MarcoFalke)
fa89ca9b5bd334813fd7e7edb202c56b35076e8d refactor: Use C++11 range based for loops to simplify rpc code (MarcoFalke)
fa459bdc87bbb050ca1c8d469023a96ed798540e rpc: Treat all args after a hidden arg as hidden as well (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
laanwj:
Code review ACK fa77de2baa40ee828c850ef4068c76cc3619e87b
fjahr:
tested ACK fa77de2baa40ee828c850ef4068c76cc3619e87b
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/19528/commits/fa77de2baa40ee828c850ef4068c76cc3619e87b
ryanofsky:
Code review ACK fa77de2baa40ee828c850ef4068c76cc3619e87b. Pretty straightfoward changes
Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3
|
|
NODE_COMPACT_FILTERS
f5c003d3ead182335252558c5c6c9b9ca8968065 [test] Add test for NODE_COMPACT_FILTER. (Jim Posen)
132b30d9c84f2a8053714a438f227b583a89a9ea [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. (Jim Posen)
b3fbc94d4f2937bb682f2766cc9a8d4fde328a3f Apply cfilters review fixups (John Newbery)
Pull request description:
If -peerblockfilters is configured, signal the `NODE_COMPACT_FILTERS` service bit to indicate that we are able to serve compact block filters, headers and checkpoints.
ACKs for top commit:
MarcoFalke:
re-review and Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065
fjahr:
Code review ACK f5c003d3ead182335252558c5c6c9b9ca8968065
clarkmoody:
Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065
ariard:
Concept and Code Review ACK f5c003d
jonatack:
ACK f5c003d3e
Tree-SHA512: 34d1c153530a0e55d09046fe548c9dc37344b5d6d50e00af1b4e1de1e7b49de770fca8471346a17c151de9fe164776296bb3dd5af331977f0c3ef1e6fc906f85
|
|
count
c133cdcdc3397a734d57e05494682bf9bf6f4c15 Cap listsinceblock target_confirmations param (Adam Stein)
Pull request description:
This addresses an issue brought up in #19587.
Currently, the `target_confirmations` parameter to `listsinceblock` is not checked for being too large. When `target_confirmations` is greater than one more than the current number of blocks, `listsinceblock` fails with error code -1. In comparison, when `target_confirmations` is less than 1, a -8 "Invalid parameter" error code is thrown.
This PR fixes the issue by returning a -8 "Invalid parameter" error if the `target_confirmations` value corresponds to a block with more confirmations than the genesis block. This happens if `target_confirmations` exceeds one more than the number of blocks.
ACKs for top commit:
laanwj:
Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15
ryanofsky:
Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15. Just suggested changes since last review. Thanks!
Tree-SHA512: 02680f4cb937d2c24d5019abd0ebfa188b8a50679a1e64e9c26bfe5c17eef6aea906832e6e2d492ba8a2ea160041bf185d66795ee691e340f6793db03c21b89a
|
|
79d6332e9e4fc01e6418247c31e31b4faa1b3b84 moveonly: Fix indentation in bumpfee RPC (Andrew Chow)
431071c28ae35be8aa012df51233be19067d625c Hide bumpfee's psbt creation behavior behind -deprecatedrpc (Andrew Chow)
4638224f64ba7c8ea7c4fb550ec89c6a6d8c7887 Add psbtbumpfee RPC (Andrew Chow)
Pull request description:
Adds a new RPC `psbtbumpfee` which always creates a psbt. `bumpfee` will then only be able to create and broadcast fee bumping transactions instead of changing its behavior based on `IsWalletSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)`.
Split from #18627
ACKs for top commit:
Sjors:
re-utACK 79d6332
meshcollider:
utACK 79d6332e9e4fc01e6418247c31e31b4faa1b3b84
fjahr:
Code review ACK 79d6332e9e4fc01e6418247c31e31b4faa1b3b84
Tree-SHA512: 1c92c4b4461bb30e78be3ee73165f624398ef33996ce36043b61a8931be667030d0fca12fd0b30097b78c56e4e9092c69582b237cbdac51d56f6be23d8c0f1bb
|
|
Previously, listsinceblock would fail with error code -1 when the
target_confirmations exceeded the number of confirmations of the genesis
block. This commit allows target_confirmations to refer to a lastblock
hash with more confirmations than exist in the chain by setting the
lastblock hash to the genesis hash in this case. This allows for
`listsinceblock "" 6` to not fail if the block count is less than 5
which may happen on regtest.
Includes update to the functional test for listsinceblock to test for
this case.
|
|
a51d0ad2de89b9757d158df95ddeba2bfcb23935 rpc: Improve addnode remove command error message (Fabian Jahr)
Pull request description:
The `addnode` RPC with the `remove` command parameter is used to remove a node from the "added nodes". It did not have test coverage and in case of failure to remove the node it responded with the confusing message "Error: Node has not been added.".
This PR adds test coverage and introduces a new error code as well as changes the error message to something that makes sense.
ACKs for top commit:
laanwj:
Code review ACK a51d0ad2de89b9757d158df95ddeba2bfcb23935
theStack:
Tested ACK https://github.com/bitcoin/bitcoin/commit/a51d0ad2de
Tree-SHA512: 033ef5de0d4d49d58ef4df3759b838c9d19ee9dfb0aff9f814a3a63d124ca231a442c930efa7d343fe1f65727c4b59fc23dd5e26fe6ea69f9e84fda48b5c5cc2
|
|
to addrman
37a480e0cd94895b6051abef12d984ff74bdc4a3 [net] Add addpeeraddress RPC method (John Newbery)
ae8051bbd8377f2458ff1f167dc30c2d5f83e317 [test] Test that getnodeaddresses() can return all known addresses (John Newbery)
f26502e9fc8a669b30717525597e3f468eaecf79 [addrman] Specify max addresses and pct when calling GetAddresses() (John Newbery)
Pull request description:
Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to `GetAddr()`. Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, `getnodeaddresses` can now return the complete addrman, which is helpful for testing and monitoring.
Also add a test-only RPC `addpeeraddress`, which adds an IP address:port to addrman. This is helpful for testing (eg #18991).
ACKs for top commit:
naumenkogs:
utACK 37a480e0cd94895b6051abef12d984ff74bdc4a3
laanwj:
Code review and lightly manually tested ACK 37a480e0cd94895b6051abef12d984ff74bdc4a3
Tree-SHA512: f86dcd410aaebaf6e9ca18ce6f23556e5e4649c1325577213d873aa09967298e65ab2dc19a72670641ae92211a923afda1fe124a82e9d2c1cad73d478ef27fdc
|
|
Allows addresses to be added to Address Manager for testing.
|
|
|
|
This also adds test coverage for the remove command which was uncovered before.
|
|
dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64 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 dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64
instagibbs:
manual inspection ACK https://github.com/bitcoin/bitcoin/pull/19674/commits/dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64
practicalswift:
ACK dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64 -- 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
|
|
expected
9e165d0de4c3cd168137fc85b8f31b371bd4e851 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:
https://github.com/bitcoin/bitcoin/blob/edec7f7c254294cd5c46ae5cf304353d458bb852/test/functional/p2p_compactblocks.py#L34-L53
Prompted by looking into: #19449
ACKs for top commit:
laanwj:
Code review ACK 9e165d0de4c3cd168137fc85b8f31b371bd4e851
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/19631/commits/9e165d0de4c3cd168137fc85b8f31b371bd4e851
Tree-SHA512: bc4a9c8bf031c8a7efb40d9625feaa3fd1f56f3b75da7034944af71ccea44328a6c708ab0c13fea85fb7cf4fd9043fe90eb94a25e95b2d42be44c2962b4904ce
|
|
fa4dfd215f62e88d668311701735c332c264fa2a 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 fa4dfd215f62e88d668311701735c332c264fa2a
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/19657/commits/fa4dfd215f62e88d668311701735c332c264fa2a :coffee:
Tree-SHA512: 36eda7eb323614a4c4f9215f1d7b40b9f9c4036d1c08eb701ea705f3e2986fdabd2fc558965a6aadabeed861034aeaeef3c00f968ca17ed7a27e42e506cda87d
|
|
566aada386e181c2ff40ef18ee660a819f485415 Test that wtxid relay peers add wtxid to reject filter (Gregory Sanders)
0fea6ede1b46f5137e8ea0fbacce169d97e4a5ee Restore test case for p2p transaction blinding (Gregory Sanders)
Pull request description:
Introduced in ca10a03addf70421893791c2c499e82fc494d60b then erroneously removed in 8d8099e97ab8af2126f6fbd223fbd82c52f2e85e. 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 566aada386e181c2ff40ef18ee660a819f485415
Tree-SHA512: be2b75b5eddb88019b79cc798f9922ca7347ccbb2210b8d4eae93fdde62e2cbb614b5247cb2fbd7ee3577dbe053875a9b62c5747aace8617f12790b8fccdeab4
|
|
9f88ded82b2898ca63d44c08072f1ba52f0e18d7 test addition of unknown segwit spends to txid reject filter (Gregory Sanders)
7989901c7eb62ca28b3d1e5d5831041a7267e495 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 9f88ded82b2898ca63d44c08072f1ba52f0e18d7 - code review
jnewbery:
Code review ACK 9f88ded82b2898ca63d44c08072f1ba52f0e18d7
ariard:
Code Review/Tested ACK 9f88ded
naumenkogs:
utACK 9f88ded82b2898ca63d44c08072f1ba52f0e18d7
jonatack:
ACK 9f88ded82b2
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
|
|
82fc4017b774aaff8799c2b6e8ba5370d94dbf4d 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 82fc4017b774aaff8799c2b6e8ba5370d94dbf4d
Tree-SHA512: 8c102b8bf831b05c5ca4b2e1feb5574dcbaed8cab0b2f22b013c5dfcb81788a38839a163dd1e2c6470ccbe5874214663b84485f45467738fd850ca38d539ae25
|
|
|
|
|
|
faa9a74c9e99eb43ba0d27fa906767ee88011aeb 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 faa9a74c9e99eb43ba0d27fa906767ee88011aeb.
Tree-SHA512: 4be850b96e23b87bc2ff42c028a5045d6f5cdbc9482ce6a6ba01cc5eb26710dab9e2ed547c363aac4bd5825151ee9996fb797261420b631bceeddbfa698d1dec
|
|
3bd67ba5a4ef3c20ef1f873b63c9f53a6c8608b6 Test addr response caching (Gleb Naumenko)
cf1569e074505dbbb9d29422803dd31bb62072d4 Add addr permission flag enabling non-cached addr sharing (Gleb Naumenko)
acd6135b43941fa51d52f5fcdb2ce944280ad01e Cache responses to addr requests (Gleb Naumenko)
7cc0e8101f01891aa8be093a00d993bb7579c385 Remove useless 2500 limit on AddrMan queries (Gleb Naumenko)
ded742bc5b96e3215d69c11fb3628d224e7ae034 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 3bd67ba5a4ef3c20ef1f873b63c9f53a6c8608b6
promag:
Code review ACK 3bd67ba5a4ef3c20ef1f873b63c9f53a6c8608b6.
ariard:
Code Review ACK 3bd67ba
Tree-SHA512: dfa5d03205c2424e40a3f8a41af9306227e1ca18beead3b3dda44aa2a082175bb1c6d929dbc7ea8e48e01aed0d50f0d54491caa1147471a2b72a46c3ca06b66f
|
|
h/t Anthony Towns
|
|
Introduced in ca10a03addf70421893791c2c499e82fc494d60b then erroneously removed in
8d8099e97ab8af2126f6fbd223fbd82c52f2e85e. The restored line is how we are
checking that the node will still re-request a specific txid given a witness-related failure.
|
|
|
|
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'>]
|
|
This is a more narrowly-construed wait which eliminates the possibility of the
wait being triggered by other messages.
Co-authored-by: Billy Garrison <billygarrison.btc@gmail.com>
|
|
|
|
|
|
|
|
once per UTXO)
82dee87933ed0714976ff4eb9657acfc13c6de84 test: test decodepsbt fee calculation (count input value only once per UTXO) (Sebastian Falbesoner)
Pull request description:
Fixes #19523, adding a simple test to `rpc_psbt.py` that checks that the decodepsbt fee matches the one given by the wallet (`walletcreatefundedpsbt`). This is in particular important for PSBTs with segwit inputs that have both a witness- and a non-witness-UTXO type set.
Example test run after reverting commit 75122780e2c46505d977e24c5612dfa9442ab754 ("Increment input value sum only once per UTXO in decodepsbt"):
```
$ test/functional/rpc_psbt.py
2020-07-26T11:31:44.862000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__sutcd4y
20.00007580
2020-07-26T11:31:47.073000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/test_framework.py", line 118, in main
self.run_test()
File "test/functional/rpc_psbt.py", line 166, in run_test
assert_equal(decoded['fee'], created_psbt['fee'])
File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/util.py", line 49, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(20.00007580 == 0.00007580)
2020-07-26T11:31:47.125000Z TestFramework (INFO): Stopping nodes
......
```
ACKs for top commit:
achow101:
ACK 82dee87933ed0714976ff4eb9657acfc13c6de84
Tree-SHA512: 296b8a701f851d482ef6200c6cbf0cf0257a79a828ac6dbc39b05d8c2d839c6fdb9d3f5a084015295cfa3eac7c11faa2f2d52e619c11627b04c75150eead8330
|
|
2c6a02e0248825e205e6deea4c38409044feb4ab Clean message_count and last_message (Troy Giorshev)
Pull request description:
From #19580
This PR changes comments to clarify the intended usage of `message_count` and `last_message`. Additionally it changes the only usage of `message_count` to use `last_message` instead, bringing the code into alignment with the intended usage.
Note: Now `message_count` is completely unused. However, it is ready to be used (i.e. the supporting code works) and likely will be used in some test in the future.
ACKs for top commit:
jnewbery:
utACK 2c6a02e0248825e205e6deea4c38409044feb4ab
Tree-SHA512: 07c7684c9586de4f845e10d7aac36c1aab9fb56b409949c1c70d5ca705bc3971ca7d5943245a0472def4efd7b4e1c5dad2f713db5ead8fca08404daf4891e98b
|
|
74507ce71eb61105fb3ae8460999099234ca7b8b walletdb: Remove BerkeleyBatch friend class from BerkeleyDatabase (Andrew Chow)
00f0041351bcd6ddbab110df1189f79ce011e192 No need to check for duplicate fileids in all dbenvs (Andrew Chow)
d86efab37002841fd059251672e1ec1a977b743f walletdb: Move Db->open to BerkeleyDatabase::Open (Andrew Chow)
4fe4b3bf1b152877677a6115f82aefaf318dd514 walletdb: track database file use as m_refcount within BerkeleyDatabase (Andrew Chow)
65fb8807ac402d1e924fd85969b5837c192bf59f Combine BerkeleyEnvironment::Verify into BerkeleyDatabase::Verify (Andrew Chow)
Pull request description:
`BerkeleyBatch` and `BerkeleyDatabase` are kind of messy. The goal of this is to clean up them up so that they are logically separated.
`BerkeleyBatch` currently handles the creation of the `BerkeleyDatabase`'s `Db` handle. This is instead moved into `BerkeleyDatabase` and is called by `BerkeleyBatch`.
Instead of having `BerkeleyEnvironment` track each database's usage, have `BerkeleyDatabase` track this usage itself with the `m_refcount` variable that is present in `WalletDatabase`.
Lastly, instead of having each `BerkeleyEnvironment` store the fileids of the databases open in it, have a global `g_fileids` to track those fileids. We were already checking fileid uniqueness globally (by checking the fileids in every environment when opening a database) so it's cleaner to do this with a global variable.
All of these changes allow us to make `BerkeleyBatch` and `BerkeleyDatabase` no longer be friend classes.
The diff of this PR is currently the same as in ##18971
Requires #19334
ACKs for top commit:
laanwj:
Code review ACK 74507ce71eb61105fb3ae8460999099234ca7b8b
ryanofsky:
Code review ACK 74507ce71eb61105fb3ae8460999099234ca7b8b. No changes since last review other than rebase
Tree-SHA512: 845d84ee1a470e2bf5d2e2e3d7738183d8ce43ddd06a0bbd57edecf5779b2f55d70728b1b57f5daab0f078650a8d60c3e19dc30b75b36e7aa952ce268399d5f6
|
|
This commit clarifies the intended usage of message_count and
last_message. Additionally it changes the only usage of message_count
to using last_message instead, bringing the code further along the
intended usage.
|
|
Checks that the RPC decodepsbt calculates the fee correctly, in particular for
PSBTs with segwit inputs that have both a witness- and a non-witness-UTXO type
set. Before commit 75122780e2c46505d977e24c5612dfa9442ab754 ("Increment input
value sum only once per UTXO in decodepsbt") the values for those inputs were
double counted.
|
|
9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5700e7a9176d0104d470b83ff9aa3589e8 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)
Pull request description:
Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.
ACKs for top commit:
MarcoFalke:
Approach re-ACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 🌾
jnewbery:
utACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8
Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
|
|
2aac093a3d60e446b85eebdf170ea6bed77bec92 test: Add test coverage for -networkactive option (Hennadii Stepanov)
3c58129b1293742a49aa196cb210ff345a7339e6 net: Log network activity status change unconditionally (Hennadii Stepanov)
62fe6aa87e4cdd8b06207abc1387c68d7bfc04c1 net: Add -networkactive option (Hennadii Stepanov)
Pull request description:
Some Bitcoin Core activity is completely local (offline), e.g., reindexing.
The `setnetworkactive` RPC command is already present. This PR adds the corresponding command-line argument / config option, and allows to start the client with disabled p2p network by providing `-networkactive=0` or `-nonetworkactive`.
This was done while reviewing #16981.
ACKs for top commit:
MarcoFalke:
re-ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92 🏠
LarryRuane:
ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92
Tree-SHA512: 446d791b46d7b556d7694df7b1f88cd4fbc09301fe4eaf036b45cb8166ed806156353cc03788a07b633d5887d5eee30a7c02a2d4307141c8ccc75e0a88145636
|
|
Since we have .walletlock in each directory, we don't need the duplicate
fileid checks across all dbenvs as it shouldn't be possible anyways.
|
|
|
|
0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb Further improve comments around recentRejects (Suhas Daftuar)
0e20cfedb704c1f76bb727e2009867d3d503a03d Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar)
cacd85209e20fc0fd08f86eed23b6ef93484ffcf test: Use wtxid relay generally in functional tests (Fabian Jahr)
8d8099e97ab8af2126f6fbd223fbd82c52f2e85e test: Add tests for wtxid tx relay in segwit test (Fabian Jahr)
9a5392fdf67f1c5c90f52d3cdb3dea4f35d1609f test: Update test framework p2p protocol version to 70016 (Fabian Jahr)
dd78d1d641178b473ab1156b71a837b9e686792b Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar)
4eb515574e1012bc8ea5dafc3042dcdf4c766f26 Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar)
97141ca442daea8fc9c307cf81a02b38dcc28fd8 Delay getdata requests from peers using txid-based relay (Suhas Daftuar)
46d78d47dea345329ba094310eec56ab00a02ddc Add p2p message "wtxidrelay" (Suhas Daftuar)
2d282e0cba9761574b6b43d134ca95f3052d7fd2 ignore non-wtxidrelay compliant invs (Anthony Towns)
ac88e2eb619821ad7ae1d45d4b40be69051d3999 Add support for tx-relay via wtxid (Suhas Daftuar)
8e68fc246d09f1e6c6dfa8c676969d97c2eb4334 Add wtxids to recentRejects instead of txids (Suhas Daftuar)
144c38582042c3b9ec53bb97ba0644fc0114f8fb Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar)
85c78d54af462996a0bca6cf97f91e1aa8778ae8 Add wtxid-index to orphan map (Suhas Daftuar)
08b39955ec7f84e835ab0b1366f0dd28dfd6ce03 Add a wtxid-index to mapRelay (Suhas Daftuar)
60f0acda713e7b9dc188aef54ef93981a93f4e44 Just pass a hash to AddInventoryKnown (Suhas Daftuar)
c7eb6b4f1fe5bd76388a023529977674534334a7 Add wtxid to mempool unbroadcast tracking (Amiti Uttarwar)
2b4b90aa8f0440deacefb5997d7bd1f9f5c591b3 Add a wtxid-index to the mempool (Suhas Daftuar)
Pull request description:
Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool.
We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in #8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this.
As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course).
Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade.
Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome.
To do items:
- [x] Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type)
- [ ] Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes.
ACKs for top commit:
naumenkogs:
utACK 0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb
laanwj:
utACK 0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb
Tree-SHA512: d8eb8f0688cf0cbe9507bf738e143edab1f595551fdfeddc2b6734686ea26e7f156b6bfde38bad8bbbe8bec1857c7223e1687f8f018de7463dde8ecaa8f450df
|
|
12410b1feb80189061eb4a2b43421e53cbb758a8 test: fix intermittent p2p_ibd_txrelay race, add test_framework.py#wait_until (Jon Atack)
Pull request description:
To fix these intermittent failures in Travis CI.
```
162/163 - p2p_ibd_txrelay.py failed, Duration: 2 s
stdout:
2020-07-19T05:44:17.213000Z TestFramework (INFO):
Check that nodes set minfilter to MAX_MONEY while still in IBD
2020-07-19T05:44:17.216000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/test_framework/test_framework.py", line 117, in main
self.run_test()
File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/p2p_ibd_txrelay.py", line 30, in run_test
assert_equal(conn_info['minfeefilter'], MAX_FEE_FILTER)
File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/test_framework/util.py", line 49, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(0E-8 == 0.09170997)
2020-07-19T05:44:17.293000Z TestFramework (INFO): Stopping nodes
```
At Marco's suggestion, cherry-picked part of #19134 to nicely simplify using `wait_until`.
ACKs for top commit:
vasild:
ACK 12410b1fe
Tree-SHA512: 615f509883682fd693e578b259cba35a9fa0bc519f1394e88c857e8b0650bfec5397bfa856cfa9e6d5ef81d0ee6ad02e4ad2b0eb0bd530b4c281cbe3e663790b
|
|
9c34aff39309b8adc99d347e07b6ddb5366498e9 Remove previous_release.sh (Brian Liotti)
e1e5960e10a9329d9f55a3967d546ffbdd896030 script: Add previous_release.py (Brian Liotti)
Pull request description:
Closes #18132
Added functionality:
1) checks file hash before untarring when using the binary download option
ACKs for top commit:
fjahr:
re-ACK 9c34aff39309b8adc99d347e07b6ddb5366498e9
Sjors:
tACK 9c34aff39309b8adc99d347e07b6ddb5366498e9
Tree-SHA512: 323f11828736a372a47f048592de8b027ddcd75b38f312dfc73f7b495d1e078bfeb384d9cdf434b3e70f2c6c0ce2da2df48e9a6460ac0e1967c6829a411c52d5
|
|
|
|
|
|
Also cleans up some doublicate lines in the rest of the test.
co-authored-by: Anthony Towns <aj@erisian.com.au>
|
|
This new p2p protocol version allows to use WTXIDs for tx relay.
|
|
Using both txid and wtxid-based relay with peers means that we could sometimes
download the same transaction twice, if announced via two different hashes from
different peers.
Use a heuristic of delaying txid-peer-getdata requests by 2 seconds, if we have
at least one wtxid-based peer.
|
|
This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
|
|
cb31ee01b42af58c64b5d3c0eabae4bcd7fd67cf [test] feefilter during and after IBD (gzhao408)
Pull request description:
This is a followup to #19204 which uses `minfeefilter=MAX_MONEY` to effectively shut off txrelay, thereby reducing inv traffic, when nodes are in IBD. It was [missing](https://github.com/bitcoin/bitcoin/pull/19204#issuecomment-644040070) a functional test.
ACKs for top commit:
jnewbery:
utACK cb31ee01b42af58c64b5d3c0eabae4bcd7fd67cf
Tree-SHA512: a9effc8193fa95fb42a2f9c66b258cc7b0941fc04c1ce3a6092f4426c9bfc7e72f702aca559b3e30e90652497f411f22fae3cf5cdb6cfd6ef6d37fed712cda67
|
|
e80259f1976545e4f1ab6a420644be0c32261773 Additionally treat Tx.nVersion as unsigned in joinpsbts (Matt Corallo)
970de70bdd3542e75b73c79b06f143168c361494 Dump transaction version as an unsigned integer in RPC/TxToUniv (Matt Corallo)
Pull request description:
Consensus-wise we already treat it as an unsigned integer (the
only rules around it are in CSV/locktime handling), but changing
the underlying data type means touching consensus code for a
simple cleanup change, which isn't really worth it.
See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299
ACKs for top commit:
sipa:
ACK e80259f1976545e4f1ab6a420644be0c32261773
practicalswift:
ACK e80259f1976545e4f1ab6a420644be0c32261773
ajtowns:
ACK e80259f1976545e4f1ab6a420644be0c32261773 code review -- checked all other uses of tx.nVersion treat it as unsigned (except for policy.cpp:IsStandard anyway), so looks good.
naumenkogs:
ACK e80259f
Tree-SHA512: 6760a2c77e24e9e1f79a336ca925f9bbca3a827ce02003c71d7f214b82ed3dea13fa7d9f87df9b9445cd58dff8b44a15571d821c876f22f8e5a372a014c9976b
|
|
Co-authored-by: Jon Atack <jon@atack.com>
|
