The flag fDIP0001ActiveAtTip shows a status of activation of DIP0001
This flag is used currently only in net.cpp for setup block buffer size.
Assume the bigger by default so far as DIP0001 is activated years ago
fa4db8671bb604e11b43a837f91de8866226f166 test: Activate all regtest softforks at height 1, unless overridden (MarcoFalke)
faad1e5ffda255aecf1b0ea2152cd4f6805e678f Introduce -testactivationheight=name@height setting (MarcoFalke)
fadb2ef2fa8561882db463f35df9b8a0e9609658 test: Add extra_args argument to TestChain100Setup constructor (MarcoFalke)
faa46986aaec69e4cf016101ae517ce8778e2ac5 test: Remove version argument from build_next_block in p2p_segwit test (MarcoFalke)
fa086ef5398b5ffded86e4f0d6633c523cb774e9 test: Remove unused ~TestChain100Setup (MarcoFalke)
Pull request description:
All softforks that are active at the tip of mainnet, should also be active from genesis in regtest. Otherwise their rules might not be enforced in user testing, thus making their testing less useful.
To still allow tests to check pre-softfork rules, a runtime argument can change the activation height.
ACKs for top commit:
laanwj:
Code review ACK fa4db8671bb604e11b43a837f91de8866226f166
theStack:
re-ACK fa4db8671bb604e11b43a837f91de8866226f166
Tree-SHA512: 6397d46ff56ebc48c007a4cda633904d6ac085bc76b4ecf83097c546c7eec93ac0c44b88083b2611b9091c8d1fb8ee1e314065de078ef15e922c015de7ade8bf
e4e7c440f4 fix: use proper chain instead using ActiveChain for test framework (Konstantin Akimov)
65b92fa093 Merge bitcoin/bitcoin#21862: test: Set regtest.BIP65Height = 111 to speed up tests (fanquake)
adcf095ab9 Merge bitcoin/bitcoin#22718: doc: Add missing PR 16333 release note (MarcoFalke)
101a863399 Merge bitcoin/bitcoin#16333: test: Set BIP34Height = 2 for regtest (MarcoFalke)
71af8816ef Merge bitcoin/bitcoin#22907: test: Avoid intermittent test failure in feature_csv_activation.py (merge-script)
fc25503cbc Merge bitcoin/bitcoin#22632: test: Set regtest.BIP66Height = 102 to speed up tests (W. J. van der Laan)
cbd2be8e18 Merge bitcoin/bitcoin#22597: consensus/params: simplify ValidDeployment check to avoid gcc warning (MarcoFalke)
8928146bfa Merge bitcoin/bitcoin#22550: test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout) (MarcoFalke)
fb00431b7c Merge bitcoin/bitcoin#22385: refactor: Use DeploymentEnabled to hide VB deployments (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Backports from bitcoin related to hard-fork mechanism and accelerated action
## What was done?
see commits for backports.
Also fixed an issue of using from ChainState on RegTest: should be used specified chain, not the "best chain".
## How Has This Been Tested?
Run unit & functional tests
## Breaking Changes
That's a breaking changes for RegTest
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK e4e7c440f4
Tree-SHA512: c02813179177781a0da3beaff65732e12017f0056565b897891b2c5a06fda125a4e032571a02fad9d8640728df82c117393bd78bc6d406e0a30f72587b2d7344
b223c3c21e89f6af76b5401413880923f7c444d6 test: Add functional test for symlinked blocks directory (laanwj)
ddb75c2e87a60ed24065bdf0c3bfabf4e058cef1 test: Add fs_tests/create_directories unit test (Hennadii Stepanov)
1f46b6e46e1454b91ff7ceb31853bc440952f8eb util: Work around libstdc++ create_directories issue (laanwj)
Pull request description:
Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than #24266, which worked around one instance of the problem.
The issue was [fixed upstream](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc), but unfortunately we'll have to carry a fix for it for a while.
This introduces a function `fs::create_directories` which wraps
`std::filesystem::create_directories`. This allows easiliy reverting the
workaround when it is no longer necessary.
ACKs for top commit:
jonatack:
re-ACK b223c3c21e89f6af76b5401413880923f7c444d6 per `git range-diff df08250 67019cd b223c3c`
hebasto:
re-ACK b223c3c21e89f6af76b5401413880923f7c444d6
w0xlt:
re-ACK b223c3c
vasild:
ACK b223c3c21e89f6af76b5401413880923f7c444d6
Tree-SHA512: 028321717c8b10d16185c3711b35da6b05fb7aa31cee1c8c7e754e92bf5a0b02719a3785cd0f6f8bf052b3bd759f644af212320672baabc9e44e0b93ba464abc
40c0e06047 fix: use coinstatsindex instead blockfilterindex in feature_prunning (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
This is follow-up changes for backports bitcoin#15946 and bitcoin#19521
It fixes failure:
TestFramework (INFO): Test invalid pruning command line options
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_node.py", line 511, in assert_start_raises_init_error
ret = self.process.wait(timeout=self.rpc_timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 1264, in wait
return self._wait(timeout=timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 2038, in _wait
raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command '['dashsrc/dashd', '-datadir=/tmp/dash_func_test_m8w6q7a2/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-mocktime=1417713337', '-uacomment=testnode0', '-logthreadnames', '-logsourcelocations', '-createwalletbackups=0', '-prune=550', '-blockfilterindex', '-mocktime=1417713337']' timed out after 120 seconds
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "dashtest/functional/feature_pruning.py", line 495, in run_test
self.test_invalid_command_line_options()
File "dashtest/functional/feature_pruning.py", line 149, in test_invalid_command_line_options
self.nodes[0].assert_start_raises_init_error(
File "dashtest/functional/test_framework/test_node.py", line 541, in assert_start_raises_init_error
self._raise_assertion_error(assert_msg)
File "dashtest/functional/test_framework/test_node.py", line 180, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] dashd should have exited within 120s with expected error Error: Prune mode is incompatible with -blockfilterindex.
TestFramework (INFO): Stopping nodes
## What was done?
Removed `blockfilterindex` test, enabled `coinstatsindex` test.
## How Has This Been Tested?
Run manually: `test/functional/feature_pruning.py`
This test is not run by our CI
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 40c0e06047
UdjinM6:
ACK 40c0e06047
Tree-SHA512: 94005c52a39cc02be8df65898844584fb247b71abab36341c3d8b4e5018902931ed8554734f27bb5d54fae2b7867ba40f136fc089fd6f9f0e57852203a5d4011
That follow-up changes for backports bitcoin#15946 and bitcoin#19521
It fixes failure:
TestFramework (INFO): Test invalid pruning command line options
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_node.py", line 511, in assert_start_raises_init_error
ret = self.process.wait(timeout=self.rpc_timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 1264, in wait
return self._wait(timeout=timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 2038, in _wait
raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command '['dashsrc/dashd', '-datadir=/tmp/dash_func_test_m8w6q7a2/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-mocktime=1417713337', '-uacomment=testnode0', '-logthreadnames', '-logsourcelocations', '-createwalletbackups=0', '-prune=550', '-blockfilterindex', '-mocktime=1417713337']' timed out after 120 seconds
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "dashtest/functional/feature_pruning.py", line 495, in run_test
self.test_invalid_command_line_options()
File "dashtest/functional/feature_pruning.py", line 149, in test_invalid_command_line_options
self.nodes[0].assert_start_raises_init_error(
File "dashtest/functional/test_framework/test_node.py", line 541, in assert_start_raises_init_error
self._raise_assertion_error(assert_msg)
File "dashtest/functional/test_framework/test_node.py", line 180, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] dashd should have exited within 120s with expected error Error: Prune mode is incompatible with -blockfilterindex.
TestFramework (INFO): Stopping nodes
c467cfffcebb30f829eeb8160166a6b941d97ed6 test: add coverage for `purpose` arg in `listlabels` (brunoerg)
Pull request description:
This PR adds test coverage for `listlabels` command when specifying the `purpose` (send and receive).
dcdfd72861/src/wallet/rpc/addresses.cpp (L698-L704)
ACKs for top commit:
kristapsk:
ACK c467cfffcebb30f829eeb8160166a6b941d97ed6
Tree-SHA512: 7e7143c1264692f7b22952e7c70dbe9ed3f5dcd2e3b69962a47be9f9c21b3f4a9089ca87962fbc8ff9116e7d2dbeb7f36d6a132c9ac13724a255cfe1b32373a8
2ef5294a5bb68ceb3797d2638567a172cc21699f rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff7b2f5e2820993443a6f868f6b0b20a test: add getblockfrompeer coverage of invalid inputs (Jon Atack)
Pull request description:
The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
Fix all issues.
ACKs for top commit:
brunoerg:
ACK 2ef5294a5bb68ceb3797d2638567a172cc21699f
Tree-SHA512: 454782cf6a44fd0e05483bb152153667ef5c8021358385ddcf89724fbbbd35e187362bdff757e00c99319527bc4c0b20c7187f67241d4585d767a29787142f25
faf7e485e901d6c72db5d969b526fa148060a003 Set regtest.BIP65Height = 111 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 65. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 65, which is enforced on mainnet for all new blocks.
ACKs for top commit:
theStack:
re-ACK faf7e485e901d6c72db5d969b526fa148060a003 📍
Zero-1729:
re-ACK faf7e485e901d6c72db5d969b526fa148060a003
kristapsk:
ACK faf7e485e901d6c72db5d969b526fa148060a003
Tree-SHA512: 79a8263e7233838666b9b636b496a8b9eb12398c779f9434677e1d62816732c0a7c7b3e73965be1fb0038d35e05e5a90e665bd74e9610104127dfc4ea38169bf
222290f54388270937cb6c174195717e2214ec0d test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c55be478f0323eafa1d560ea2c56f04fb23 test: Create all blocks with version 4 or higher (MarcoFalke)
Pull request description:
BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.
I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)
This pull is done in two commits:
* test: Create all blocks with version 4 or higher:
Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.
* test: Set BIP34Height = 2 for regtest:
This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed
ACKs for top commit:
ajtowns:
ACK 222290f54388270937cb6c174195717e2214ec0d
jonatack:
ACK 222290f54388270937cb6c174195717e2214ec0d tested and reviewed rebased to current master 5e213822f86d
theStack:
Tested ACK 222290f54388270937cb6c174195717e2214ec0d
Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
fa676dbac87919061de9f82bce65e373e8d85bd1 test: pep-8 whitespace (MarcoFalke)
faed284eabb250a07331dfca22bb8f96a95c72ea test: Avoid intermittent test failure in feature_csv_activation.py (MarcoFalke)
Pull request description:
Otherwise there will be disconnects if the test runs longer than the default peertimeout (60s):
```
node0 2021-09-05T20:28:30.973116Z (mocktime: 2021-09-01T07:17:29Z) [net] [net.cpp:1323] [InactivityCheck] socket receive timeout: 393061s peer=0
```
Fix that by skipping `InactivityCheck` via a large `-peertimeout`.
ACKs for top commit:
fanquake:
ACK fa676dbac87919061de9f82bce65e373e8d85bd1
Tree-SHA512: 061c0585a805aa2f8e55c4beedd4b8498a2951f33d60aa3632dda0a284db3a627d14a23dbd57e8a66c69a1612f39418e3a755c8ca97f6ae1105c0d70f0d1a801
fafe896a0b870d85250927bd5374caf73d379468 test: Set regtest.BIP66Height = 102 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 66. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 66, which is enforced on mainnet for all new blocks.
ACKs for top commit:
GeneFerneau:
Concept + code review ACK [fafe896](fafe896a0b)
0xB10C:
crACK fafe896a0b870d85250927bd5374caf73d379468
laanwj:
ACK fafe896a0b870d85250927bd5374caf73d379468
Zero-1729:
tACK fafe896
kristapsk:
ACK fafe896a0b870d85250927bd5374caf73d379468. Full functional test suite showed few second speed incrase on my laptop (although I didn't do proper benchmarking with multiple runs, just single `time ./test/functional/test_runner.py` on current master vs this PR).
theStack:
Tested ACK fafe896a0b870d85250927bd5374caf73d379468
hg333:
tACK fafe896a0b
Tree-SHA512: 4bbee3c8587d612e74a59fde49b6439c1296f2fc27d3a7cf59a35e920f729fdd581c930290bd04def618f81412236676ddb99b4ceb4d80dfb9fd610b128a04b1
f4cb0fbfe1 fix: no need to relay quorum commitment in case of block undo (Konstantin Akimov)
0431a33919 fix: follow-up changes for bitcoin#14193. (Konstantin Akimov)
86b76d19b6 Merge bitcoin/bitcoin#21812: ci: Enable D_GLIBCXX_DEBUG for multiprocess task (fanquake)
334496ea7e Merge bitcoin/bitcoin#21775: p2p: Limit m_block_inv_mutex (MarcoFalke)
23b83109ea Merge bitcoin/bitcoin#21750: net: remove unnecessary check of CNode::cs_vSend (MarcoFalke)
b34514191f Merge bitcoin/bitcoin#21738: test: Use clang-12 for ASAN, Add missing suppression (fanquake)
3411577473 Merge bitcoin/bitcoin#19160: multiprocess: Add basic spawn and IPC support (W. J. van der Laan)
970048d917 fix: missing changes from bitcoin#19267 - run multiprocess on CI (Konstantin Akimov)
f2b7ee73db fix: follow-up bitcoin#15402 - removed dead code (Konstantin Akimov)
274068cdbc fix: follow-up bitcoin/bitcoin#21732 - minor missing typo (MarcoFalke)
e9450a8b36 Merge #21669: test: Remove spurious double lock tsan suppressions by bumping to clang-12 (MarcoFalke)
ef92c3065c Merge #21663: ci: Fix macOS brew install command (W. J. van der Laan)
Pull request description:
## Issue being fixed or feature implemented
Just regular backports from v22
## What was done?
See commits for backports.
Also there're 2 bugs are fixed which became visible after backporting bitcoin#21775 - both are related to possible deadlocks in net_processing
## How Has This Been Tested?
Run unit and functional tests. Enabled multiprocess builds on CI
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK f4cb0fbfe1
PastaPastaPasta:
utACK f4cb0fbfe1
Tree-SHA512: 3204c2aa243fa4834ccf4ff4672d183cf9b35f87b857df8543572cd685729e15fca39f85b27194233e57cbc1746e36b556efab95ce20d0aa0a7d4476a9f3c6c0
92834d1ef2 fix: help p2p_timeouts to succeed on the my localhost (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Locally on my environment the functional tests `p2p_timeouts.py` fails in 80% runs.
Output:
```
stdout:
2024-08-08T05:02:32.216000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:35.079000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
'''
2024-08-08T05:02:35.080000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "DASH/test/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "DASH/test/functional/p2p_timeouts.py", line 93, in run_test
no_verack_node.wait_for_disconnect(timeout=1)
File "DASH/test/functional/test_framework/p2p.py", line 495, in wait_for_disconnect
self.wait_until(test_function, timeout=timeout, check_connected=False)
File "DASH/test/functional/test_framework/p2p.py", line 487, in wait_until
wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "DASH/test/functional/test_framework/util.py", line 267, in wait_until_helper
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
''' not true after 1.0 seconds
2024-08-08T05:02:35.581000Z TestFramework (INFO): Stopping nodes
2024-08-08T05:02:36.582000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0/test_framework.log
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Hint: Call DASH/test/functional/combine_logs.py '/tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0' to consolidate all logs
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-08-08T05:02:36.582000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
stderr:
TEST | STATUS | DURATION
p2p_timeouts.py | ✓ Passed | 4 s
p2p_timeouts.py | ✖ Failed | 4 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 7 s
ALL | ✖ Failed | 37 s (accumulated)
Runtime: 7 s
```
## What was done?
Increased a timeout to see for first disconnect event +1 second.
## How Has This Been Tested?
100% succeed:
```
$ test/functional/test_runner.py -j20 p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py
p2p_timeouts.py | ✓ Passed | 4 s
p2p_timeouts.py | ✓ Passed | 5 s
p2p_timeouts.py | ✓ Passed | 5 s
p2p_timeouts.py | ✓ Passed | 6 s
p2p_timeouts.py | ✓ Passed | 6 s
p2p_timeouts.py | ✓ Passed | 7 s
p2p_timeouts.py | ✓ Passed | 7 s
p2p_timeouts.py | ✓ Passed | 8 s
p2p_timeouts.py | ✓ Passed | 8 s
p2p_timeouts.py | ✓ Passed | 9 s
p2p_timeouts.py | ✓ Passed | 9 s
p2p_timeouts.py | ✓ Passed | 10 s
p2p_timeouts.py | ✓ Passed | 10 s
p2p_timeouts.py | ✓ Passed | 11 s
p2p_timeouts.py | ✓ Passed | 11 s
p2p_timeouts.py | ✓ Passed | 12 s
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
ACK 92834d1ef2
PastaPastaPasta:
utACK 92834d1ef2
Tree-SHA512: 598178fd97e82def16b32cbaf1f476d3416768456a7f92fb4faadc041b73147cc7be3e6760287bde22d1a3e5a5a9190124ede6da81a1722feba1e80fcc3ae4e3
bitcoin#25454 introduces a 10 point penalty for remitting more than
MAX_BLOCKS_TO_ANNOUNCE unconnected block headers. Whether they are
connected or not is determined by taking the first entry and running
its hashPrevBlock through LookupBlockIndex. This new behaviour causes a
test failure in p2p_dos_header_tree.py in Dash.
Bitcoin doesn't face a test failure with this new behaviour as the first
non-fork block in its test data is the dump for block 1 (00000000b873e7
9784647a6c82962c70d228557d24a747ea4d1b8bbe878e1206) but Dash uses block
0 (00000bafbc94add76cb75e2ec92894837288a481e5c005f6563d91623bf8bc2c),
the genesis block.
By definition of a genesis block, it has a hashPrevBlock of 0, which
cannot be looked up. This trips the penalty. This doesn't cause any
problems in the field as nobody is expected to ever broadcast the
genesis block but it does cause a test failure for us.
We need to correct that by getting rid of the genesis block from the
test data.
12f094ec215aacf30e4e380c0399f80d4e45c345 test: use constants for CSV/CLTV activation heights in rpc_signrawtransaction (Sebastian Falbesoner)
746f203f1950a7df50b9a7de87a361cc7354ffb4 test: introduce `generate_to_height` helper, use in rpc_signrawtransaction (Sebastian Falbesoner)
e3237b1cd07a5099fbb0108218194eb653b6a9f3 test: check that CSV/CLTV are active in rpc_signrawtransaction (Sebastian Falbesoner)
Pull request description:
This PR primarily aims to solve the current RPC timeout problem for test rpc_signrawtransaction.py, as described in #22542. In the course of that the test is also improved in other ways (see https://github.com/bitcoin/bitcoin/pull/22542#pullrequestreview-714297804).
Reviewers guideline:
* In `test_signing_with_cltv()`, a comment is fixed -- it wrongly referred to CSV, it should be CLTV.
* As preparation, assertions are added that ensure that CSV and CLTV have been really activated after generating blocks by checking the 'softforks' output of the getblockchaininfo() RPC. Right now in master, one could remove (or decrease, like in #22542) the generate calls and the test would still pass, when it shouldn't.
* A helper `generate_to_height()` is introduced which improves the previous way of reaching a block height in two ways:
- instead of blindly generating TH blocks to reach target block height >= TH, the current block height CH is taken into account, and only (TH - CH) are generated in total
- to avoid potential RPC timeouts, the block generation is split up into multiple generatetoaddress RPC calls ([as suggested by laanwj](https://github.com/bitcoin/bitcoin/pull/22542#issuecomment-886237866)); here chunks of 200 blocks have been chosen
* The helper is used in the affected sub-tests, which should both speed-up the test (from ~18s to ~12s on my machine) and avoid potential timeouts
* Finally, the activation constants for CSV and CLTV are used instead of using magic numbers 500 and 1500
Open questions:
* Any good naming alternatives for `generate_to_height()`? Not really happy with the name, happy to hear suggestions
* Where to put the CSV and CLTV activation height constants in the test_framewor folder? I guess importing constants from other tests isn't really the desired way to go
ACKs for top commit:
laanwj:
Code review and tested ACK 12f094ec215aacf30e4e380c0399f80d4e45c345
rajarshimaitra:
reACK 12f094ec21
Tree-SHA512: 14509f6d3e5a5a05d6a30a3145bb82cd96a29d9d8a589abf1944a8bf34291cde78ce711195f52e9426588dc822b3618ec9b455e057360021ae46152bb7613516
stdout:
2024-08-08T05:02:32.216000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:35.079000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
'''
2024-08-08T05:02:35.080000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "DASH/test/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "DASH/test/functional/p2p_timeouts.py", line 93, in run_test
no_verack_node.wait_for_disconnect(timeout=1)
File "DASH/test/functional/test_framework/p2p.py", line 495, in wait_for_disconnect
self.wait_until(test_function, timeout=timeout, check_connected=False)
File "DASH/test/functional/test_framework/p2p.py", line 487, in wait_until
wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "DASH/test/functional/test_framework/util.py", line 267, in wait_until_helper
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
''' not true after 1.0 seconds
2024-08-08T05:02:35.581000Z TestFramework (INFO): Stopping nodes
2024-08-08T05:02:36.582000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0/test_framework.log
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Hint: Call DASH/test/functional/combine_logs.py '/tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0' to consolidate all logs
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-08-08T05:02:36.582000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
stderr:
TEST | STATUS | DURATION
p2p_timeouts.py | ✓ Passed | 4 s
p2p_timeouts.py | ✖ Failed | 4 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 7 s
ALL | ✖ Failed | 37 s (accumulated)
Runtime: 7 s
38ecd6f951 test: reduce BRRHeight on regtest (Odysseas Gabrielides)
Pull request description:
## Issue being fixed or feature implemented
In regtest, Block Reward Reallocation is buried at height 2500, which happens after v20 activation.
## What was done?
Reduced BRRHeight in regtest from 2500 to 1000.
The purpose of this change is to simplify regtests of Platform as well.
Note: This change affects only regtest.
## How Has This Been Tested?
tests
## Breaking Changes
no, this only affects Regtest (where we make no guarantees about breaking stuff)
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: b5a1c2b2c2b70682f266d9d0af9e048a03417c0cb2480eb5ab5838965342b6465acd10d8dac5a0d3c5c5f59f4e36ac5b909a838bc3805c2265a83776e92b4827
0f239203a8 partial bitcoin#24169: Add --enable-c++20 option (Kittywhiskers Van Gogh)
a3b79267e0 merge bitcoin#20744: Use std::filesystem. Remove Boost Filesystem & System (Kittywhiskers Van Gogh)
be7ac493d0 merge bitcoin#24167: consistently use fsbridge:: for ifstream / ofstream (Kittywhiskers Van Gogh)
7ffea4348f merge bitcoin#24104: Make compatible with boost 1.78 (Kittywhiskers Van Gogh)
7c270e6883 merge bitcoin#24026: Block unsafe std::string fs::path conversion copy_file calls (Kittywhiskers Van Gogh)
b0d2484a0b merge bitcoin#23522: Improve fs::PathToString documentation (Kittywhiskers Van Gogh)
20d359b570 partial bitcoin#23469: Remove Boost build note from build-unix.md (Kittywhiskers Van Gogh)
193f6fde2e merge bitcoin#23446: Mention that BerkeleyDB is for legacy wallet in build-unix (Kittywhiskers Van Gogh)
ecfac10b8e merge bitcoin#22937: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method (Kittywhiskers Van Gogh)
23fe7e2f07 chore: dashify symbols in some unit tests (Kittywhiskers Van Gogh)
28b96a071d merge bitcoin#22840: fix unoptimized libraries in depends (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on https://github.com/dashpay/dash/pull/6085
* Depends on https://github.com/dashpay/dash/pull/6137
* Dependency for https://github.com/dashpay/dash/pull/6150
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 0f239203a8
PastaPastaPasta:
utACK 0f239203a8
Tree-SHA512: 4b8f0ae55185ece27d8084a5339196b7ed993c8138f4c59a0db3e16729d4edf4a59a68a8c7309c32df57734c07182821c4878b55c253da5df763204ad7158426
21f174aff1 feat: improve query categorisation in Qt App (Konstantin Akimov)
c863473286 test: add spending asset unlock tx in functional tests (Konstantin Akimov)
1fb67ece0e feat: make a support of Qt app to show Platform Transfer transaction as a new type of transaction (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Transfers from platform have incorrectly shown amount in Dash Core wallet app.
They also shown in Qt app as self-send that is not completely true.
## What was done?
Added new type of transaction to Qt App, added a filter for its type, fixed calculation of output for tx records.
As well added a new type of transaction `platform-transfer` in rpc output of `gettransaction` RPC
## How Has This Been Tested?
Make a Platform Transfer transaction on RegTest and check it in Dash Core
Helper to see it: export dpath=/tmp/dash_func_test_PATHPATH/ ; src/qt/dash-qt -regtest -conf=$dpath/node0/dash.conf -datadir=$dpath/node0/ -debug=0 -debuglogfile=/dev/stdout
## Breaking Changes
There's new type of transaction "platform-transfer" in rpc output of `gettransaction`.
**This PR DOES NOT change any consensus rules.**
Breaking changes that makes withdrawal transaction immature is moved to https://github.com/dashpay/dash/pull/6128
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
Top commit has no ACKs.
Tree-SHA512: ec2a54a910f121ad30ff8e94cf17080b5b3c651872e9bc3de9ec0924ca7f7a0e526b74b05cde26aaf860e3809e67f66142112319a69c216527e5bcb1b8a2b8f6
f22ade31b9 tests: more strict test for withrawal 1000 and minor improvements (UdjinM6)
4ad18f64f5 fix: properly test hard limit of 1000 dash (Konstantin Akimov)
a2fe2b27d9 test: minor improvements for credit pool functional test (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
DIP for Credit Pool says:
```
The withdrawal should not be mined if:
* It requests more DASH than the credit pool contains
* It requests more than 1000 DASH
* The credit pool contains more than 1000 DASH, and the withdrawal would result in more than a 10% reduction in the credit pool over the 576-block window
* The credit pool contains less than 1000 DASH, and the withdrawal would result in more than 100 DASH being removed from the pool over the 576-block window
```
Though, current functional test for asset locks improperly test this case, because threshold for big withdrawal happens by 10%, not 1000 dash.
## What was done?
Improvements for functional asset lock test to actually test a limit 1000 dash, not just 10%
## How Has This Been Tested?
See changes
## Breaking Changes
N/A, changes only for tests
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK f22ade31b9
Tree-SHA512: 2fdbfa85a3fc41683d68d1577916178ad686ccf0fba6abb22dc84a7ad69e0d44f876e371a24935c5167baa5491000662cc98cc1cd205e3817f0ffc65d2b4953d
b73f48f3b9 Merge bitcoin/bitcoin#22057: test: use MiniWallet (P2PK mode) for feature_dersig.py (MarcoFalke)
d5a8d5e6a0 Merge bitcoin/bitcoin#22048: test: MiniWallet: introduce enum type for output mode (MarcoFalke)
f4cd20b115 Merge bitcoin/bitcoin#21945: test: add P2PK support to MiniWallet (MarcoFalke)
7be6db6dca docs: add an explanation for vsize in MiniWallet (Konstantin Akimov)
5d10b41302 Merge bitcoin/bitcoin#21840: test: Misc refactor to get rid of &foo[0] raw pointers (MarcoFalke)
7522ee9868 Merge bitcoin/bitcoin#21900: test: use MiniWallet for feature_csv_activation.py (MarcoFalke)
c6f603c26f Merge bitcoin/bitcoin#21897: rpc: adjust incorrect RPCHelpMan types (MarcoFalke)
1dffe3ab9f Merge bitcoin/bitcoin#21867: test: use MiniWallet for p2p_blocksonly.py (MarcoFalke)
81d21eea14 Merge #21557: test: small cleanup in RPCNestedTests tests (MarcoFalke)
cc169c2457 partial Merge #20842: docs: consolidate typo & url fixing (MarcoFalke)
2be1604405 Merge #20459: rpc: Fail to return undocumented return values (MarcoFalke)
Pull request description:
## What was done?
Backports from v22 bitcoin.
Mostly related to MiniWallet and RPC improvements, see commits
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK b73f48f3b9
PastaPastaPasta:
utACK b73f48f3b9
Tree-SHA512: 588f3a30697c0d77dadcc463aba71a00bf26eeef41b0cb8b9197799a217ebeb1d1ce7b5021ccc4576f0e9ca0e75ad840820cdc682fe8f120596788a528727a0b
764b3a3239 test: disable mocktime in p2p_eviction.py (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
No idea why CI has no issues but `p2p_eviction.py` fails locally after #6103 (my guess is that it's because P2PInterface can't work with mocktime properly).
## What was done?
Disable mocktime in `p2p_eviction.py`
## How Has This Been Tested?
Run tests locally
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: a9be9032c7697ff47b2256395f0fb126deeccd9bee6f101a71a1f88e1f25b08fa039ed5eb4cd4b1b308e8136d64510a544b7019ed9147ea2e80f8cb83ff25412
84934bf70e11fe4cda1cfda60113a54895d4fdd5 multiprocess: Add echoipc RPC method and test (Russell Yanofsky)
7d76cf667eff512043a28d4407cc89f58796c42b multiprocess: Add comments and documentation (Russell Yanofsky)
ddf7ecc8dfc64cf121099fb047e1ac871de94f4c multiprocess: Add bitcoin-node process spawning support (Russell Yanofsky)
10afdf0280fa93bfffb0a7665c60dc155cd84514 multiprocess: Add Ipc interface implementation (Russell Yanofsky)
745c9cebd50fea1664efef571dc1ee1bddc96102 multiprocess: Add Ipc and Init interface definitions (Russell Yanofsky)
5d62d7f6cd48bbc4e9f37ecc369f38d5e1e0036c Update libmultiprocess library (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR adds basic process spawning and IPC method call support to `bitcoin-node` executables built with `--enable-multiprocess`[*].
These changes are used in https://github.com/bitcoin/bitcoin/pull/10102 to let node, gui, and wallet functionality run in different processes, and extended in https://github.com/bitcoin/bitcoin/pull/19460 and https://github.com/bitcoin/bitcoin/pull/19461 after that to allow gui and wallet processes to be started and stopped independently and connect to the node over a socket.
These changes can also be used to implement new functionality outside the `bitcoin-node` process like external indexes or pluggable transports (https://github.com/bitcoin/bitcoin/pull/18988). The `Ipc::spawnProcess` and `Ipc::serveProcess` methods added here are entry points for spawning a child process and serving a parent process, and being able to make bidirectional, multithreaded method calls between the processes. A simple example of this is implemented in commit "Add echoipc RPC method and test."
Changes in this PR aside from the echo test were originally part of #10102, but have been split and moved here for easier review, and so they can be used for other applications like external plugins.
Additional notes about this PR can be found at https://bitcoincore.reviews/19160
[*] Note: the `--enable-multiprocess` feature is still experimental, and not enabled by default, and not yet supported on windows. More information can be found in [doc/multiprocess.md](https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md)
ACKs for top commit:
fjahr:
re-ACK 84934bf70e11fe4cda1cfda60113a54895d4fdd5
ariard:
ACK 84934bf. Changes since last ACK fixes the silent merge conflict about `EnsureAnyNodeContext()`. Rebuilt and checked again debug command `echoipc`.
Tree-SHA512: 52a948b5e18a26d7d7a09b83003eaae9b1ed2981978c36c959fe9a55abf70ae6a627c4ff913a3428be17400a3dace30c58b5057fa75c319662c3be98f19810c6
3e05a57297ddc9c55604a41e50a7a94d220db7ee test: use MiniWallet (P2PK mode) for feature_dersig.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_dersig.py) to be run even with the Bitcoin Core wallet disabled. A valid DER-signature is created by using the recently introduced P2PK-Mode of the MiniWallet (#21945).
ACKs for top commit:
MarcoFalke:
cr ACK 3e05a57297ddc9c55604a41e50a7a94d220db7ee
Tree-SHA512: 0fb8da8ed8b47f68bcb57301eb4f0171a6c9e44539b7554626969347e5d6f80b3b9085f2cc160cd038a990f0d81b8b614846260fbed43b5f950d77f1b7aa81cf
6cebac598e5e85eadd60eb1274d7f33d63ce1108 test: MiniWallet: introduce enum type for output mode (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #21945 which lifted the number of MiniWallet's tx output modes from 2 to 3 (by adding P2PK Support).
Since the current way of specifying the mode on the ctor via two booleans is ugly and error-prone (see table in comment https://github.com/bitcoin/bitcoin/pull/21945#issuecomment-842526575), a new Enum type `MiniWalletMode` is introduced that can hold the following values:
- ADDRESS_OP_TRUE
- RAW_OP_TRUE
- RAW_P2PK
Also adds documentation that should guide the user on which mode is useful for what etc. with a summary table. (Can also be split up in a separate commit or shortened if that is desired, maybe it's considered to be too verbose).
ACKs for top commit:
MarcoFalke:
cr ACK 6cebac598e5e85eadd60eb1274d7f33d63ce1108
Tree-SHA512: cbbc10806d9d9e62829548094415e9f1a281cd247b9a9fc7f7f33b923c723aa03e7bc3024623a77fb1f7da4d73455fa8244840f746980d32acdad97ee12100da
4bea30169218e2f21e0c93a059966b41c8edd205 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
dc7eb64e83f5b8e63f12729d5f77b1c920b136e4 test: MiniWallet: add P2PK support (Sebastian Falbesoner)
Pull request description:
This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/21900#discussion_r629524841). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed.
Possible follow-ups:
* Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
* Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
* Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)
ACKs for top commit:
laanwj:
Code review ACK 4bea30169218e2f21e0c93a059966b41c8edd205
Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
bd7f27d16dacf6f7de3b4f6bd052def41d9601be refactor: feature_csv_activation.py: move tx helper functions to methods (Sebastian Falbesoner)
2eca46b0aa0ecf4738500b53523d7013985b387d test: use MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_csv_activation.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Short reviewers guideline:
- Since we exclusively work with anyone-can-spend outputs here (raw scriptPubKey = OP_TRUE), signing is not needed anymore. The function `sign_transaction` and its calls are removed, after changing a tx (e.g. its scriptSig or nVersion) a simple `.rehash()` call is sufficient. Also, generating an address `self.nodeaddress` (and with that, passing it to the the various test tx creation/sending helper methods) is not needed anymore and removed.
- The test repeatedly uses the same input for creating different txs (e.g. with different txversions 1 and 2). To let `MiniWallet` create a tx with a specific input, we have to call `.get_utxo()` before which also marks the UTXO as spent. The method is changed to also support keeping the UTXO in its internal list (`mark_as_spent=False`). With the behaviour on master, the second call to `.get_utxo()` with the same input would fail.
- To keep the diff in the first commit short, the `miniwallet` is set as a global variable, to avoid passing it on every tx creation/spending helper. The global is eliminated in the second (refactoring) commit, where all the helpers are moved to the test class as methods. By that, we can use `self.nodes[0]` directly in the helpers and don't have to pass it again and again. I think there could still be a lot of improvements/refactoring done in the test, but that should hopefully serve as a good basis.
ACKs for top commit:
laanwj:
Code review ACK bd7f27d16dacf6f7de3b4f6bd052def41d9601be
MarcoFalke:
review ACK bd7f27d16dacf6f7de3b4f6bd052def41d9601be 🐕
Tree-SHA512: 24fb6a0f7702bae40d5271d197119827067d4b597e954d182e4c1aa5d0fa870368eb3ffed469b26713fa8ff8eb3ecc06abc80b2449cd68156d5559e7ae8a2b11
9f767e84381d678ed24e3f7f981976f9da34971e test: use MiniWallet for p2p_blocksonly.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_blocksonly.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Note that MiniWallet creates segwit transactions by default, i.e. txid and wtxid are not identical and we have to return both from `check_p2p_tx_violation(...)`: wtxid is needed to match an expected `"received getdata for: wtx ..."` debug output, whereas the txid is needed to wait for a certain tx via `wait_for_tx(...)`.
ACKs for top commit:
jonatack:
ACK 9f767e84381d678ed24e3f7f981976f9da34971e tested with `--disable-wallet`
Tree-SHA512: f08001f02c3c310ccdf713af0ba17304368a36414f412749908bbe8c03ad1e902190b8768b79f3b4909855762f285e7ab1b627cc4f45c90b42bb097a43cb4318
BACKPORT NOTICE:
missing changes in src/test/validation_tests.cpp (signet)
1112035d32ffe73a4522226c8cb2f6a5878d3ada doc: fix various typos (Ikko Ashimine)
e8640849c775efcf202dbd34736fed8d61379c49 doc: Use https URLs where possible (Sawyer Billings)
Pull request description:
Consolidates / fixes the changes from #20762, #20836, #20810. There is no output when `test/lint/lint-all.sh` is run.
Closes#20807.
ACKs for top commit:
MarcoFalke:
ACK 1112035d32ffe73a4522226c8cb2f6a5878d3ada
Tree-SHA512: 22ca824688758281a74e5ebc6a84a358142351434e34c88c6b36045d2d241ab95fd0958565fd2060f98317e62e683323b5320cc7ec13592bf340e6922294ed78
fa5362a9a0c5665c1a4de51c3ce4758c93a9449e rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs (MarcoFalke)
Pull request description:
Wallet RPCs that allow a rescan based on block-timestamp or block-height
need to sync with the active chain first, because the user might assume
the wallet is up-to-date with the latest block they got reported via a
blockchain RPC.
ACKs for top commit:
meshcollider:
utACK fa5362a9a0c5665c1a4de51c3ce4758c93a9449e
Tree-SHA512: d4831f1f08f854f9a49fc969de86c438f856e41c2163c801a6ff36dc2f6299cb342b44663279c524a8b7ca9a50895db1243cd7d49bed79277ada857213f20a26
7e32fde912b3924fdb27ec1f658ac11fcf160b3e test: feature_cltv.py: don't return tx copies in modification functions (Sebastian Falbesoner)
9ab2ce0a6673acc7ee0f85158fc087fce0fc7dd8 test: drop unused node parameters in feature_cltv.py (Sebastian Falbesoner)
0c2139a3f160d1d443460e4c5928109a6ab8cd11 test: fix typo in feature_cltv.py (s/ctlv/cltv/) (Sebastian Falbesoner)
Pull request description:
This tiny PR cleans up the test `feature_cltv.py` in the following ways:
* fixes a typo (s/ctlv/cltv/); compared to CHECKLOCKTIMEVERIFY, CHECKTIMELOCKVERIFY probably also sounds good and you [even get some search results for it](https://www.google.com/search?q=%22CHECKTIMELOCKVERIFY%22), but it's still wrong ;)
* drops the unused "node" parameters from the tx modification functions
* don't return a copy from the tx modification functions; it's modified in-place, hence a copy is not needed and `cltv_validate(tx, ...)` looks more natural than `tx = cltv_validate(tx, ...)`
ACKs for top commit:
MarcoFalke:
review ACK 7e32fde912b3924fdb27ec1f658ac11fcf160b3e 📼
Tree-SHA512: d2e6230977442f6a511d0f7c99431a44ad3a423647f4f327ce2ce8efe78bf9616c0d2093f5e3c3550f690dcb3f625ddf53227505c01ced70227425f249c25364
0d9fdd329e81cb171d687042290f4e6b1507d7f4 test, doc: refer to the correct variable names in p2p_invalid_tx.py (aitorjs)
Pull request description:
_tx_orphan_no_fee_ and _tx_orphan_invalid_ don't exist as transactions.
Have been replaced by _tx_orphan_2_no_fee_ and _tx_orphan_2_invalid_ respectively.
**Motivation**: Comments are more accurate and easy understandable under the tests context (I think).
ACKs for top commit:
kristapsk:
utACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4
theStack:
ACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4 📃
Tree-SHA512: a4cafd931e51fe2a67085e10e9c61178c864c14982664d112b76327e040af08cd1de04eca4a8ae980fad57ba7078017ce02fc60e7658f38380e8172c2ae28b77
127b4608e9dbb8217c74c9332e82fcec8c326fa8 test: Check if specified config file cannot be opened (nthumann)
6bb54708e6457f21596793a7149dc6dfea1dc871 util: Check if specified config file cannot be opened (nthumann)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22612.
When running e.g. `./src/bitcoind -datadir=/tmp/bitcoin -regtest -conf=/tmp/bitcoin/regtest/bitcoin.conf` and the specified config cannot be opened (doesn't exist, permission denied, ...), the initialization silently uses the default config.
As voidburn already noted:
> I can't think of a situation in which a config file is specified explicitly (in the startup options, as per service unit linked above), but inaccessible, where the fail condition should be to keep booting using defaults instead.
With this patch applied, the initialization will fail immediately, if the specified config file cannot be opened. If no config file is explicitly specified, the behavior is unchanged. This not only affects `bitcoind`, but also `bitcoin-cli` and `bitcoin-qt`.
In the example below the datadir is accessible, but the config file is not due to insufficient permissions:
```
$ ./src/bitcoind -datadir=/tmp/bitcoin -regtest --debug=1 -conf=/tmp/bitcoin/regtest/bitcoin.conf
Error: Error reading configuration file: specified config file "/tmp/bitcoin/regtest/bitcoin.conf" could not be opened.
```
ACKs for top commit:
0xB10C:
ACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Zero-1729:
tACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
theStack:
Tested ACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Tree-SHA512: 4fe487921485426f1d1da8d256c388af517b984b639d776aec7b159b3e23b669824093d3bdd31139d9415ed5f5de405b3e6a51b110c8ab471f12b9c99ac67cc1
b01cd9471f435bb36b8ed5211a56baad51111ad2 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc19814743cb12960a99793197c811e2750a06b test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c4826529a2d30ffc850bce4d44da6019b test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202c4820b1ad5c375d3d671fd0a18e092 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)
Pull request description:
The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:
f8462a6d27/test/functional/feature_cltv.py (L26-L35)
This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
* _the stack is empty_
➡️ prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
* _the top item on the stack is less than 0_
➡️ prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
* _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
* _the top stack item is greater than the transaction's nLockTime field_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
* _the nSequence field of the txin is 0xffffffff_
➡️ prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500
The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").
ACKs for top commit:
MarcoFalke:
review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣
Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
06e1fb0b170a69996a7ce1ef5203785a7bc6b278 Add new format string placeholders for walletnotify to include relevant block information for transactions (Maayan Keshet)
Pull request description:
This patch includes two new format placeholders for walletnotify:
%b - the hash of the block containting the transaction (zeroed if a mempool transaction)
%h - the height of the block containing the transaction (zero if a mempool transaction)
I've included test suite changes to check and validate the above functional requirements as well as doc/help description changes.
**Motivation**
The walletnotify option is used to be notified of new transactions relevant to the wallet of the node.
A common usage pattern is to perform afterwards additional RPC calls to determine:
1. If this is a mempool transaction or not (i.e. are there any confirmations?)
2. What block was it included in?
3. Did this transaction was seen before and is now seen again because of a fork?
All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer.
Please let me know of any questions and suggestions.
ACKs for top commit:
laanwj:
ACK 06e1fb0b170a69996a7ce1ef5203785a7bc6b278
Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775
bfc083e9b7 Merge #21574: Drop JSONRPCRequest constructors after #21366 (MarcoFalke)
c0cdb0488b Merge #21572: Fix wrong wallet RPC context set after #21366 (MarcoFalke)
2f7814acdd Merge #21035: Remove pointer cast in CRPCTable::dumpArgMap (MarcoFalke)
d3b1ef374c refactor: simplify implementation of RPC composite commands (Konstantin Akimov)
3270becc9b chore: add TODO to make client parsing for composite commands (Konstantin Akimov)
d55759fa79 Merge #20012: rpc: Remove duplicate name and argNames from CRPCCommand (Wladimir J. van der Laan)
1d87ce4e86 Merge #18531: rpc: remove deprecated CRPCCommand constructor (MarcoFalke)
a7e538d7ae fix: missing changes from bitcoin#19250 (Konstantin Akimov)
68c5da41dc feat: fix help message - all subcommands support it now! (Konstantin Akimov)
d3e181f516 fix: add missing client's argument parsing for RPC commands (Konstantin Akimov)
37bd4009c1 refactor: use monostate instead std::optional in CoreContext (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Backports from bitcoin v22 rpc command related
## What was done?
See commits for backports.
Also:
- refactored and significantly simplified implementation of composite commands
- added missing changes from bitcoin#19250
- fix help message for rpc `help` - all subcommands support it now
- add missing client's argument parsing for RPC commands
- CoreContext uses std::monostate instead nullopt which is best-practice for std::variant
## How Has This Been Tested?
Run unit/functional tests.
Checked autocomplete for various commands
Checked help for various commands
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK bfc083e9b7
UdjinM6:
utACK bfc083e9b7
Tree-SHA512: b7b586eac2f848a6808c677252a5965577bc783778fd70d3025b8d510113de2b177d423d72ea5f61ddd8905673bf3458e55810ada371ee235fbaa19de8d2d36f
69c37f4ec2 rpc: make sure `upgradetohd` always has the passphrase for `UpgradeToHD` (Kittywhiskers Van Gogh)
619b640a77 wallet: unify HD chain generation in CWallet (Kittywhiskers Van Gogh)
163d31861c wallet: unify HD chain generation in LegacyScriptPubKeyMan (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
When filming demo footage for https://github.com/dashpay/dash/pull/6093, I realized that if I tried to create an encrypted blank legacy wallet and run `upgradetohd [mnemonic]`, the client would crash.
```
dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash'
dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed.
Posix Signal: Aborted
No debug information available for stacktrace. You should add debug information and then run:
dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa===
```
The expected set of operations when performing privileged operations is to first use `walletpassphrase [passphrase] [time]` to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., `upgradetohd` imports **always** need the passphrase, if encrypted).
You might have noticed that I used `upgradetohd [mnemonic]` instead of the correct syntax, `upgradetohd [mnemonic] "" [passphrase]` that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told.
```
Error: Please enter the wallet passphrase with walletpassphrase first.
```
Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so `EnsureWalletIsUnlocked()` is happy, so now the following happens.
```
upgradetohd()
| Insufficient validation has allowed us to supply a blank passphrase
| for an encrypted wallet
|- CWallet::UpgradeToHD()
|- CWallet::GenerateNewHDChainEncrypted()
| We get our hands on vMasterKey by generating the key from our passphrase
| and using it to unlock vCryptedMasterKey.
|
| There's one small problem, we don't know if the output of CCrypter::Decrypt
| isn't just gibberish. Since we don't have a passphrase, whatever came from
| CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the
| vMasterKey we just got is gibberish
|- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain()
|- res = LegacyScriptPubKeyMan::EncryptHDChain()
| |- EncryptSecret()
| |- CCrypter::SetKey()
| This is where everything unravels, the gibberish key's size doesn't
| match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out.
|- assert(res)
We assume are inputs are safe so there's no real reason we should crash.
Except our inputs aren't safe, so we crash. Welp! :c
```
This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (19512988c6), trying to do the same thing would return you a vague error
```
Failed to generate encrypted HD wallet (code -4)
```
In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to `upgradetohd`, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash.
```
upgradetohd()
| We've been supplied a passphrase, so we can try and validate it by
| trying to unlock the wallet with it. If it fails, we know we got the
| wrong passphrase.
|- CWallet::Unlock()
| | Before we bother unlocking the wallet, we should check if we're
| | already unlocked, if we are, we can just say "unlock successful".
| |- CWallet::IsLocked()
| | Wallet is indeed unlocked.
| |- return true;
| The validation method we just tried to use has a bail-out mechanism
| that we don't account for, the "unlock" succeded so I guess we have the
| right passphrase.
[...] (continue call chain as mentioned earlier)
|- assert(res)
Oh...
```
This pull request aims to resolve crashes caused by the above two edge cases.
## Additional Information
As this PR was required me to add additional guardrails on `GenerateNewCryptedHDChain()` and `GenerateNewHDChainEncrypted()`, it was taken as an opportunity to resolve a TODO ([source](9456d0761d/src/wallet/wallet.cpp (L5028-L5038))). The following mitigations have been implemented.
* Validating `vMasterKey` size (any key not of `WALLET_CRYPTO_KEY_SIZE` size cannot be used for encryption and so, cannot be a valid key)
* Validating `secureWalletPassphrase`'s presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase)
* Using `Unlock()` to validate the correctness of `vMasterKey`. (the two other instances of iterating through `mapMasterKeys` use `Unlock()`, see [here](1394c41c8d/src/wallet/wallet.cpp (L5498-L5500)) and [here](1394c41c8d/src/wallet/wallet.cpp (L429-L431)))
* `Lock()`'ing the wallet before `Unlock()`'ing the wallet to avoid the `IsLocked()` bail-out condition and then restoring to the previous lock state afterwards.
* Add an `IsCrypted()` check to see if `upgradetohd`'s `walletpassphrase` is allowed to be empty.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 69c37f4ec2
UdjinM6:
utACK 69c37f4ec2
PastaPastaPasta:
utACK 69c37f4ec2
Tree-SHA512: 4bda1f7155511447d6672bbaa22b909f5e2fc7efd1fd8ae1c61e0cdbbf3f6c28f6e8c1a8fe2a270fdedff7279322c93bf0f8e01890aff556fb17288ef6907b3e
earlier it was possible to make it all the way to `EncryptSecret`
without actually having the passphrase in hand until being told off
by `CCrypter::SetKey`, we should avoid that.
also, let's get rid of checks that `UpgradeToHD` is now taking
responsibility for. no point in checking if the wallet is unlocked
as it has no bearing on your ability to upgrade the wallet.
c72ec70fdf feat: implement governance RPCs votealias and votemany for descriptor wallets (Konstantin Akimov)
490832959d refactor: new method to generate a signing message in CGovernanceVote (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
RPCs `governance votemany` and `governance votealias` use forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as
```
test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
```
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## What was done?
Use CWallet's interfaces instead LegacyScriptPubKeyMan
## How Has This Been Tested?
Functional tests `feature_governance.py` and `feature_governance_cl.py` to run by both ways - legacy and descriptor wallets.
Run unit and functional tests.
Extra test done locally:
```diff
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
if self.options.descriptors is None:
# Prefer BDB unless it isn't available
- if self.is_bdb_compiled():
- self.options.descriptors = False
- elif self.is_sqlite_compiled():
+ if self.is_sqlite_compiled():
self.options.descriptors = True
+ elif self.is_bdb_compiled():
+ self.options.descriptors = False
```
to flip flag descriptor wallets/legacy wallets for all functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK c72ec70fdf
PastaPastaPasta:
utACK c72ec70fdf
Tree-SHA512: 2c18f0d4acb1c4d57da81bf54f0d155682f558eeb7271df7e6fe75c126ef7f047562794a6730e3ca5351abc4e2daded06b874c2ab77f9c47b840c89d8a158c9f
fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204 rpc: Remove duplicate name and argNames from CRPCCommand (MarcoFalke)
fa92912b4bb4629addcbfdfb7cc000be701614af rpc: Use RPCHelpMan for check-rpc-mappings linter (MarcoFalke)
faf835680be39811827504f77005b6603165f53e rpc: [refactor] Use concise C++11 code in CRPCConvertTable constructor (MarcoFalke)
Pull request description:
Currently, the RPC argument names are specified twice to simplify consistency linting. To avoid having to specify the argnames twice when adding new arguments, remove the linter and add an equivalent test based on RPCHelpMan.
ACKs for top commit:
laanwj:
ACK fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204
Tree-SHA512: 3f5f32f5a09b22d879f24aa67031639d2612cff481d6aebc6cfe6fd757cafb3e7bf72120b30466f59292a260747b71e57322c189d5478b668519b9f32fcde31a
2db69d7b81 chore: add release notes for "quorum platformsign" (Konstantin Akimov)
283c5f89a2 feat: create new composite command "quorum platformsign" (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
It splits from #6100
With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only.
## What was done?
Implemented a new composite command "quorum platformsign"
This composite command let to limit quorum type for signing for case of whitelist.
After that old way to limit platform commands can be deprecated - #6105
## How Has This Been Tested?
Updated a functional tests to use platform signing for Asset Unlocks feature.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 2db69d7b81
PastaPastaPasta:
utACK 2db69d7b81
Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb
85abbb97b4 chore: add release notes for composite command for whitelist (Konstantin Akimov)
78ad778bb0 feat: test composite commands in functional test for whitelist (Konstantin Akimov)
a102a59787 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/66https://github.com/dashpay/dash-issues/issues/65
## What was done?
Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see https://github.com/dashpay/dash/pull/6052https://github.com/dashpay/dash/pull/6051https://github.com/dashpay/dash/pull/6055 and other related PRs
This PR makes whitelist feature to be compatible with composite commands.
Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things.
Platform at their side can use config such as this one (created based on shumkov's example):
```
rpc: {
host: '127.0.0.1',
port: 9998,
users: [
{
user: 'dashmate',
password: 'rpcpassword',
whitelist: null,
lowPriority: false,
},
{
username: 'platform-dapi',
password: 'rpcpassword',
whitelist: [],
lowPriority: true,
},
{
username: 'platform-drive-consensus',
password: 'rpcpassword',
whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
lowPriority: false,
},
{
username: 'platform-drive-other',
password: 'rpcpassword',
whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
],
lowPriority: true,
},
],
allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'],
},
```
## How Has This Been Tested?
Updated functional tests, see commits
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
LGTM, utACK 85abbb97b4
PastaPastaPasta:
utACK 85abbb97b4
Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b
1840c9441a fix: drop extra pings - follow up for #18638 (UdjinM6)
264e7f9e62 Merge #18638: net: Use mockable time for ping/pong, add tests (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Split from https://github.com/dashpay/dash/pull/6102
## What was done?
So far as bitcoin#19499 is backported, bitcoin#18638 can be finished and removed related workarounds.
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 1840c9441a
PastaPastaPasta:
utACK 1840c9441a
Tree-SHA512: f34657c14514d6ee596175b3faf9ee44e58e8b0339939a0708d58ab2d119786830c183f9b236ed87c08ef8e1dbd031a38fc596b5aa4d38e10521658df4330e79
d3e842f605 refactor: remove dead code which has no use since composite commands are refactored (Konstantin Akimov)
58c5d431fe fix: follow-up to #6017 - enable one more assert in wallet_descriptor test (Konstantin Akimov)
dbed4a31af fix: update comment for wallet_keypool_hd due to bitcoin#17681 DNM (Konstantin Akimov)
4741bcc5c3 chore: remove outdated todo - removed by bitcoin#16898 (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Multiple TODO is reviewed and fixes in this PR
## What was done?
See commits
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK d3e842f605
PastaPastaPasta:
utACK d3e842f605
Tree-SHA512: 25080abcce8fa3dbb4e4c71d321b24cb9d23c073e6caf75366be58623023af50d28bc1cdac4098eb78a87b4fe0f3c33d39bb06257f0590b3e42e5fcad161ea2d
0133c9866d feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06769 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1e96 refactor: replace static private member method to static method (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.
This PR aims to fix this issue, as requested by shumkov for needs of platform:
> mailformed signature and can’t verify signature due to unknown quorum is the same error?
> possible to distingush ?
## What was done?
Return enum in CL verifying code `chainlock_handler.VerifyChainLock`.
The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}`
## How Has This Been Tested?
Functional test `feature_llmq_chainlocks.py` is updated
## Breaking Changes
`submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 0133c9866d
PastaPastaPasta:
utACK 0133c9866d
Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2fa1153c6d76efc8113fa01b06943ece util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
createwallet has changed list of arguments: createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )
load_on_startup used to be an argument 5 but now has a number 6.
Both arguments 5 and 6 are boolean and it can confuse an user.
To prevent confusion if user is not aware about this breaking changes,
the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup.
This requirement can be removed when major amount of users updated to v21
6c5246803d test: add tests for both current and future behaviour (UdjinM6)
4e86bda4dc feat: stricter bestCLHeightDiff checks (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Current `bestCLHeightDiff` checks are too relaxed
## What was done?
Make`bestCLHeightDiff` checks stricter
## How Has This Been Tested?
Run a node on mainnet/testet, run tests
## Breaking Changes
Old nodes aren't aware of this new logic so it's activated via `mn_rr` hardfork
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 6c5246803d
Tree-SHA512: 2028d0ceb00a2270c92831ef38488d009d0bac47be4fc6a23ac93efdcf74847f1b9e99a529863fb4e14c65f120adda4e12c5b9e084d0f667d5f0fbaf80e3701d
b23d94b14f partial bitcoin#23706: getblockfrompeer followups (Kittywhiskers Van Gogh)
7e5cc5e375 merge bitcoin#23702: Add missing optional to getblockfrompeer (Kittywhiskers Van Gogh)
c294457b52 merge bitcoin#20295: getblockfrompeer (Kittywhiskers Van Gogh)
63ac87f011 merge bitcoin#22722: update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (Kittywhiskers Van Gogh)
07e4c2cdd0 merge bitcoin#21934: Include versionbits signalling details during LOCKED_IN (Kittywhiskers Van Gogh)
960e7687d4 merge bitcoin#19651: importdescriptors update existing (Kittywhiskers Van Gogh)
1f31823fed merge bitcoin#21910: remove redundant fOnlySafe argument (Kittywhiskers Van Gogh)
69c5aa8947 merge bitcoin#21359: include_unsafe option for fundrawtransaction (Kittywhiskers Van Gogh)
169dce7e50 merge bitcoin#20286: deprecate addresses and reqSigs from rpc outputs (Kittywhiskers Van Gogh)
7cddf70c58 merge bitcoin#20867: Support up to 20 keys for multisig under Segwit context (Kittywhiskers Van Gogh)
7c59923845 merge bitcoin#18344: Fix nit in getblockchaininfo (Kittywhiskers Van Gogh)
ec0803a0f5 refactor: align `TxToUniv` argument list with upstream (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Closes https://github.com/dashpay/dash/issues/6000
* `TxToUniv`'s argument list needed to be rearranged to match upstream as closely as possible (i.e. placing Dash-specific arguments at the end of the list to allow for code to be backported unmodified, relying on default arguments instead of having to modify each invocation to insert the default argument in between).
This was due to a new `TxToUniv` variant being introduced in [bitcoin#20286](https://github.com/bitcoin/bitcoin/pull/20286)
* The maximum number of public keys in a multisig remains the same. The upper limit for bare multisigs is and always has been `MAX_PUBKEYS_PER_MULTISIG` ([source](19512988c6/src/script/interpreter.cpp (L1143-L1144))), which has always been 20 ([source](19512988c6/src/script/script.h (L28-L29))). The limit of up to 16 comes from P2SH overhead ([source](19512988c6/src/script/descriptor.cpp (L877-L880))) and that hasn't changed.
In effect, what [bitcoin#20867](https://github.com/bitcoin/bitcoin/pull/20867) does to Dash Core is change the error from "too many public keys" (as we'll be testing against the bare multisig limit) to "excessive redeemScript size" ([source](19512988c6/src/rpc/util.cpp (L223-L225))) (which is the _true_ limitation).
* Backporting [bitcoin#21934](https://github.com/bitcoin/bitcoin/pull/21934) required a minor change in the condition needed to emit `activation_height` as `has_signal` in the preceding block will evaluate true for both `STARTED` and `LOCKED_IN` while earlier it would only for `STARTED`.
* In [bitcoin#22722](https://github.com/bitcoin/bitcoin/pull/22722), a `self.stop_node` had to be added to account for the absurd fee warning that a new test condition introduces.
* [bitcoin#23706](https://github.com/bitcoin/bitcoin/pull/23706) is partial due to commits in it that rely on RPC type enforcement, which is currently not implemented in Dash Core.
* `feature_dip3_deterministicmns.py` depends on the reporting of `addresses` to validate multisigs containing expected payees. There is no replacement RPC call to report the pubkeys that compose a multisig address. In the interm, the `-deprecatedrpc=addresses` flag has been enabled to allow the test to run otherwise unmodified.
## Breaking Changes
* The following RPCs: `gettxout`, `getrawtransaction`, `decoderawtransaction`, `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`, `/rest/block` deprecated the following fields (which are no longer returned in the responses by default): `addresses`, `reqSigs`.
The `-deprecatedrpc=addresses` flag must be passed for these fields to be included in the RPC response. Note that these fields are attributes of the `scriptPubKey` object returned in the RPC response. However, in the response of `decodescript` these fields are top-level attributes, and included again as attributes of the `scriptPubKey` object.
* When creating a hex-encoded Dash transaction using the `dash-tx` utility with the `-json` option set, the following fields: `addresses`, `reqSigs` are no longer returned in the tx output of the response.
* The error message for attempts at making multisigs with >16 pubkeys will change to an "excessive redeemScript size" instead of the previous "too many public keys".
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK b23d94b14f
PastaPastaPasta:
utACK b23d94b14f
Tree-SHA512: 659d9ba3a0a9f8594b307a7056ab172309d5a0d9efec605bc4b345f7e0fb1032ad303af9e8f51dbd6549e82d0b738ad41eae8a5b3aebf59081f39df0d4b5ec7c
c9a600e0fa fix: linkage error - message signer better to be common code rather than libconsensus (Konstantin Akimov)
8299b3b369 feat: protxregistar implementation for descriptor wallets (Konstantin Akimov)
6f45432f76 refactor: removed unused SignSpecialTxPayloadByString (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
RPC `protx updateregistar` uses forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as
```
test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
```
## What was done?
New method `SignSpecialTxPayloadByHash` is implemented in interface instead exporting raw private key for some address.
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## How Has This Been Tested?
Functional test `feature_dip3_deterministicmns.py` to run by both ways - legacy and descriptor wallets.
Run unit and functional tests.
Extra test done locally:
```diff
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
if self.options.descriptors is None:
# Prefer BDB unless it isn't available
- if self.is_bdb_compiled():
- self.options.descriptors = False
- elif self.is_sqlite_compiled():
+ if self.is_sqlite_compiled():
self.options.descriptors = True
+ elif self.is_bdb_compiled():
+ self.options.descriptors = False
```
to flip flag descriptor wallets/legacy wallets for all functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK c9a600e0fa
Tree-SHA512: 00aea1cd9db3537b9a9dcdee096d47ea48337edeea3f52ad54aea91781678b8641ac2dd86b67f61f87e3912945bcb5361a42a3279b6c08bb8d9f096bed8fe842
3971613285 feat: functional tests for RPC getgovernanceinfo (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/63
## What was done?
It adds functional test for `getgovernanceinfo`
## How Has This Been Tested?
Run unit/functional tests
Check output of `test/functional/test_runner.py -j20 --previous-releases --coverage --extended`
```
Uncovered RPC commands:
- cleardiscouraged
- debug
- getblockheaders
- getmerkleblocks
- getpoolinfo
- voteraw
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 3971613285
Tree-SHA512: f2d93db9068a707f2b33dd841b14f266db226fc5e907f19a49cf7a856b840f0c2c1f521c462996888933bb0ed636c288db12b1d36978c124cb536ea1ab9f3892
241f073932 feat: rpc external users are comma separated list (Konstantin Akimov)
68def970ad refactor: re-order arguments options alphabetically (Konstantin Akimov)
c7efd56a07 feat: rpc external users: use 2 queues but no extra threads (Konstantin Akimov)
c575a5808a feat: change handler to '/' for external users, use only rpc user name to choose queue (Konstantin Akimov)
f1c1fd873e feat: implementation for /external handler for RPC (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
To avoid struggling to response to critical rpc requests, and split them from 3rd parties who uses a node as an external service, there are introduced one more queue of requests that will be served without throttling for instance consensus important rpcs
## What was done?
new command line arguments:
- `rpcexternaluser` - List of comma-separated usernames for JSON-RPC external connections. If not specified, there's no special queue is created, all requests in one queue
- `rpcexternalworkqueue=<n>` - Set the depth of the work queue to service external RPC calls
## How Has This Been Tested?
Functional test `rpc_platform_filter.py` is updated to test new functionality
## Breaking Changes
NA
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 241f073932
PastaPastaPasta:
utACK 241f073932
Tree-SHA512: 15b371f24f5302853b85419e2b20c29749d6aae1c98a541d7471f1d3a681643063302c2a5ecce04dfad2da9101ea69d2f08a7e0e11a28609c6011d78273c57a7
18328279ec fix: force mnsync to skip gov obj sync on reconnection (UdjinM6)
08331bb950 fix: apply suggestions (UdjinM6)
3c3489d7a1 test: add test (UdjinM6)
41ab95dbf8 feat: skip governance checks for blocks below the best chainlock (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
A node can miss governance trigger sometimes and then it would stuck not being able to sync any further. This issue can be fixed manually by resetting sync status and reconsidering the "invalid" block. However, that's inconvenient. Also, what it does under the hood is it simply disables some parts of block validation. We could do that automagically and more precise if we would trust ChainLocks instead.
## What was done?
Skip governance checks for blocks below the best known chainlock, add tests.
## How Has This Been Tested?
Run tests.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 18328279ec
PastaPastaPasta:
utACK 18328279ec
Tree-SHA512: 3cc4e2707e24b36c9f64502561667d0cb66eced7019db7941781ab1b84cfd267b3dab4684c71b059e074450ea76dc8e342744bffdd1ca1be6ccceb34b3580659
faa137eb9eac5554504b062a6dc865ca87fd572b test: Speed up rpc_blockchain.py by removing miniwallet.generate() (MarcoFalke)
fa1fe80c757df0adcbfaf41b5c5c8a468bc07b6f test: Change address type from P2PKH to P2WSH in rpc_blockchain (MarcoFalke)
fa4d8f3169e38cbdbae20258efebe7070c49f522 test: Cache 25 mature coins for ADDRESS_BCRT1_P2WSH_OP_TRUE (MarcoFalke)
fad25153f5c8e88f72cf666b16b0b0dbdc45d3b1 test: Remove unused bug workaround (MarcoFalke)
faabce7d07c5776e4116b1a7ad1f6c408a4a4e46 test: Start only the number of nodes that are needed (MarcoFalke)
Pull request description:
Speed up various tests:
* Remove unused nodes, which only consume time on start/stop
* Remove unused "bug workarounds"
* Remove the need for `miniwallet.generate()` by adding `miniwallet.scan_blocks()`. (On my system, with valgrind, generating 105 blocks takes 3.31 seconds. Rescanning 5 blocks takes 0.11 seconds.)
ACKs for top commit:
laanwj:
Code review ACK faa137eb9eac5554504b062a6dc865ca87fd572b
Tree-SHA512: ead1988d5aaa748ef9f8520af1e0bf812cf1d72e281ad22fbd172b7306d850053040526f8adbcec0b9a971c697a0ee7ee8962684644d65b791663eedd505a025
fa61b9d1a68820758f9540653920deaeae6abe79 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24a36b62df35d12ecf6c6370671568c8 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac17f93decd4ee38c956e7aa55983f0d test: Add tests (MarcoFalke)
fac05ccdade8b34c969b9cd9b37b355bc0aabf9c wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a68820758f9540653920deaeae6abe79
fjahr:
Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
faaad1bbac46cfeb22654b4c59f0aac7a680c03a p2p: Ignore version msgs after initial version msg (MarcoFalke)
fad68afcff731153d1c83f7f56c91ecbb264b59a p2p: Ignore non-version msgs before version msg (MarcoFalke)
Pull request description:
Handshake misbehaviour doesn't cost us more than any other unknown message, so it seems odd to treat it differently
ACKs for top commit:
jnewbery:
utACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a
practicalswift:
ACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a: patch looks correct
Tree-SHA512: 9f30c3b5c1f6604fd02cff878f10999956152419a3dd9825f8267cbdeff7d06787418b41c7fde8a00a5e557fe89204546e05d5689042dbf7b07fbb7eb95cddff
fb8a4db8f6 Merge bitcoin/bitcoin#26717: test: Improve `check-doc.py` pattern (MarcoFalke)
349cad2865 Merge bitcoin/bitcoin#26708: clang-tidy: Fix `modernize-use-nullptr` in headers (MarcoFalke)
6bf786d168 Merge bitcoin/bitcoin#25735: net: remove useless call to IsReachable() from CConnman::Bind() (fanquake)
012b0b7169 Merge bitcoin/bitcoin#24258: test: check localaddresses in getnetworkinfo for nodes with proxy (MarcoFalke)
c67f527b0b Merge bitcoin-core/gui#448: Add helper to load font (Hennadii Stepanov)
8e0abeb1c1 Merge bitcoin-core/gui#345: Connection Type Translator Comments (Hennadii Stepanov)
688b66e9d1 Merge bitcoin-core/gui#266: Doc: Copyright: Fix embedded font file location (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Trivial backports
## What was done?
## How Has This Been Tested?
Built and ran tests locally; p2p_addr_relay.py fails locally. Not sure why
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK fb8a4db8f6
knst:
utACK fb8a4db8f6
Tree-SHA512: abb9469f25c6d45acea01da6d2b9cb6df94822f61d06f80e3008b32d2522016370a1e0b0c9ff95b92df22c4f227fc40f7765d76f1987eac7603155fe2d894593
89bb25d22a0e1c700dba4e3b754984c9b2b14836 test: check localaddresses in getnetworkinfo for nodes with proxy (brunoerg)
Pull request description:
This PR adds test coverage for the field `localaddresses` for `getnetworkinfo`. In this case, it verifies if this field is empty for all nodes since they are using proxy.
Reference:
515200298b/src/init.cpp (L449)
ACKs for top commit:
jonatack:
ACK 89bb25d22a0e1c700dba4e3b754984c9b2b14836
Tree-SHA512: 3c765c7060b6972c1ae5a1104734cd7669b650b5f6aa4f623f4299567732260da5083fef306a7c1e71c931f5d1396f24abad251d95c3d82b1f3ee0efee7fcd1f
1cbf3b9a53 merge bitcoin-core/gui#206: Display fRelayTxes and bip152_highbandwidth_{to, from} in peer details (Kittywhiskers Van Gogh)
239062192e merge bitcoin#20764: cli -netinfo peer connections dashboard updates (Kittywhiskers Van Gogh)
06a6f8444c merge bitcoin#25147: follow ups to #20799 (removing support for v1 compact blocks) (Kittywhiskers Van Gogh)
6274a571b7 merge bitcoin#20799: Only support version 2 compact blocks (Kittywhiskers Van Gogh)
f4ce573538 merge bitcoin#22340: Use legacy relaying to download blocks in blocks-only mode (Kittywhiskers Van Gogh)
73b8f84fdb merge bitcoin#22147: p2p: Protect last outbound HB compact block peer (Kittywhiskers Van Gogh)
2ce481849a merge bitcoin#20599: Tolerate sendheaders and sendcmpct messages before verack (Kittywhiskers Van Gogh)
799214b2c8 merge bitcoin#19776: expose high bandwidth mode state via getpeerinfo (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Version 2 of BIP152 high-bandwidth mode/compact blocks implements SegWit support.
As Dash does not implement SegWit, there has never been a need to implement v2 (and therefore, have all the code necessary to support both v1 and v2, that gets removed as part of making support v2 only).
* Despite that, the changes surrounding removing support for both versions (that in our case, do not apply as we never have supported v2) refactor the code in other ways and influence their behaviour. In the interest of upstream alignment, those changes have been backported.
* [bitcoin#19776](https://github.com/bitcoin/bitcoin/pull/19776) doesn't seem to work on its own without successive backports, specifically [bitcoin#20799](https://github.com/bitcoin/bitcoin/pull/20799), despite the latter being a later backport.
<details>
<summary>19776-only p2p_compactblocks.py run (9f2c868947cc254d021e1a9bd00eb7bc80061e81)</summary>
```
dash@825a14c32b73:/src/dash$ ./test/functional/p2p_compactblocks.py
2024-06-09T12:29:09.777000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_kb2nr5oe
2024-06-09T12:29:16.341000Z TestFramework (INFO): Testing SENDCMPCT p2p message...
2024-06-09T12:29:31.432000Z TestFramework (INFO): Testing compactblock construction...
2024-06-09T12:29:40.068000Z TestFramework (INFO): Testing compactblock requests...
2024-06-09T12:29:44.597000Z TestFramework (INFO): Testing getblocktxn handler...
2024-06-09T12:29:59.808000Z TestFramework (INFO): Testing compactblock requests/announcements not at chain tip...
2024-06-09T12:30:03.855000Z TestFramework (INFO): Testing handling of incorrect blocktxn responses...
2024-06-09T12:30:05.868000Z TestFramework (INFO): Testing reconstructing compact blocks from all peers...
2024-06-09T12:30:09.389000Z TestFramework (INFO): Testing end-to-end block relay...
2024-06-09T12:30:10.404000Z TestFramework (INFO): Testing handling of invalid compact blocks...
2024-06-09T12:30:12.418000Z TestFramework (INFO): Testing invalid index in cmpctblock message...
2024-06-09T12:30:14.384000Z TestFramework (INFO): Testing high-bandwidth mode states via getpeerinfo...
2024-06-09T12:30:16.893000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/src/dash/test/functional/test_framework/test_framework.py", line 158, in main
self.run_test()
File "./test/functional/p2p_compactblocks.py", line 849, in run_test
self.test_highbandwidth_mode_states_via_getpeerinfo()
File "./test/functional/p2p_compactblocks.py", line 791, in test_highbandwidth_mode_states_via_getpeerinfo
hb_test_node.send_and_ping(msg_block(block))
File "/src/dash/test/functional/test_framework/p2p.py", line 579, in send_and_ping
self.sync_with_ping(timeout=timeout)
File "/src/dash/test/functional/test_framework/p2p.py", line 596, in sync_with_ping
self.wait_until(test_function, timeout=timeout)
File "/src/dash/test/functional/test_framework/p2p.py", line 487, in wait_until
wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "/src/dash/test/functional/test_framework/util.py", line 249, in wait_until_helper
if predicate():
File "/src/dash/test/functional/test_framework/p2p.py", line 484, in test_function
assert self.is_connected
AssertionError
2024-06-09T12:30:17.396000Z TestFramework (INFO): Stopping nodes
2024-06-09T12:30:18.400000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_kb2nr5oe
2024-06-09T12:30:18.401000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_kb2nr5oe/test_framework.log
2024-06-09T12:30:18.401000Z TestFramework (ERROR):
2024-06-09T12:30:18.401000Z TestFramework (ERROR): Hint: Call /src/dash/test/functional/combine_logs.py '/tmp/dash_func_test_kb2nr5oe' to consolidate all logs
2024-06-09T12:30:18.401000Z TestFramework (ERROR):
2024-06-09T12:30:18.401000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-06-09T12:30:18.402000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-06-09T12:30:18.402000Z TestFramework (ERROR):
```
</details>
<details>
<summary>20799-incl p2p_compactblocks.py run (aa116c4f0b4753b615f9483aa03adec5ee4fd655)</summary>
```
dash@825a14c32b73:/src/dash$ ./test/functional/p2p_compactblocks.py
2024-06-09T12:34:27.169000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_7d65lmhz
2024-06-09T12:34:32.695000Z TestFramework (INFO): Testing SENDCMPCT p2p message...
2024-06-09T12:34:51.288000Z TestFramework (INFO): Testing compactblock construction...
2024-06-09T12:34:55.325000Z TestFramework (INFO): Testing compactblock requests...
2024-06-09T12:34:59.861000Z TestFramework (INFO): Testing getblocktxn handler...
2024-06-09T12:35:07.460000Z TestFramework (INFO): Testing compactblock requests/announcements not at chain tip...
2024-06-09T12:35:09.503000Z TestFramework (INFO): Testing handling of incorrect blocktxn responses...
2024-06-09T12:35:11.519000Z TestFramework (INFO): Testing reconstructing compact blocks from all peers...
2024-06-09T12:35:15.039000Z TestFramework (INFO): Testing end-to-end block relay...
2024-06-09T12:35:16.055000Z TestFramework (INFO): Testing handling of invalid compact blocks...
2024-06-09T12:35:17.062000Z TestFramework (INFO): Testing invalid index in cmpctblock message...
2024-06-09T12:35:19.139000Z TestFramework (INFO): Testing high-bandwidth mode states via getpeerinfo...
2024-06-09T12:35:22.159000Z TestFramework (INFO): Stopping nodes
2024-06-09T12:35:23.163000Z TestFramework (INFO): Cleaning up /tmp/dash_func_test_7d65lmhz on exit
2024-06-09T12:35:23.163000Z TestFramework (INFO): Tests successful
```
</details>
* The backport of [bitcoin-core/gui#206](https://github.com/bitcoin-core/gui/pull/206) is a continuation of 3e8ba24c87 from [dash#5964](https://github.com/dashpay/dash/pull/5964)
* The backport of [bitcoin#20764](https://github.com/bitcoin/bitcoin/pull/20764) is a continuation of bd934c71eb from [dash#6034](https://github.com/dashpay/dash/pull/6034)
## Breaking changes
* The `getpeerinfo` RPC returns two new boolean fields, `bip152_hb_to` and `bip152_hb_from`, that respectively indicate whether we selected a peer to be in compact blocks high-bandwidth mode or whether a peer selected us as a compact blocks high-bandwidth peer.
High-bandwidth peers send new block announcements via a `cmpctblock` message rather than the usual inv/headers announcements. See BIP 152 for more details.
* Blocks-only mode will use legacy relaying instead of BIP152 high-bandwidth mode
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 1cbf3b9a53
PastaPastaPasta:
utACK 1cbf3b9a53
knst:
utACK 1cbf3b9a53
Tree-SHA512: 5947b622d8d57a1dc9445cd6e07d4ad690379416d0fcf04ed574269975d1beb704691a79ff081341f3c800cf11869d401f1ed90baa5449f371f9ce658f2d2e95
a93fec6f2d merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging (Kittywhiskers Van Gogh)
d1a4b14b48 merge bitcoin#23354: Introduce new V4 format addrman (Kittywhiskers Van Gogh)
7a97aabfe0 test: restore pre-bitcoin#23306 tests to validate port distinguishment (Kittywhiskers Van Gogh)
1a050d6cb4 merge bitcoin#23306: Make AddrMan support multiple ports per IP (Kittywhiskers Van Gogh)
d56702aa0c merge bitcoin#23140: Make CAddrman::Select_ select buckets, not positions, first (Kittywhiskers Van Gogh)
19b0145379 merge bitcoin#22839: improve addrman logging (Kittywhiskers Van Gogh)
3910c68028 merge bitcoin#23053: Use public methods in addrman fuzz tests (Kittywhiskers Van Gogh)
b6ec8ab6df merge bitcoin#22950: Pimpl AddrMan to abstract implementation details (Kittywhiskers Van Gogh)
236cf36d88 merge bitcoin#22734: Avoid crash on corrupt data, Force Check after deserialize (Kittywhiskers Van Gogh)
2420ac9e53 merge bitcoin#23041: Add addrman deserialization error tests (Kittywhiskers Van Gogh)
8aefa9b93a merge bitcoin#22762: Raise InitError when peers.dat is invalid or corrupted (Kittywhiskers Van Gogh)
c9a645f814 merge bitcoin#22879: Fix format string in deserialize error (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6040.
* Dash already introduced support for storage of address-port pairs (referred to as "port discrimination") and allowed the usage of non-default ports in P2P with [dash#2168](https://github.com/dashpay/dash/pull/2168).
* Albeit this was only permitted for networks with `fAllowMultiplePorts` enabled (which at the time was `devnet` and as it stands currently, on every network except `mainnet`).
* Keeping in line with the above policy (discussion on lifting such restrictions on `mainnet` is outside the scope of this PR), when backporting [bitcoin#23306](https://github.com/bitcoin/bitcoin/pull/23306), changes have been made to retain the effects of `discriminate_ports`.
* This involves appending placing a `!m_discriminate_ports` condition to behaviour that otherwise would be _removed_ entirely.
* Additionally, in line with upstream backports, changes have been made that render port distinguishment _enabled_ as the new default in `addrman_tests` (the old default was to keep it _disabled_, to mirror `mainnet` and pre-change upstream behaviour).
* To ensure distinguishment _disabled_ works as expected, affected pre-backport tests were reintroduced with the `_nondiscriminate` suffix.
---
I would propose at some point to rename the flag to `ignore_port`/`suppress_port` (if not remove it altogether should the `mainnet` restriction be lifted) as discriminate (or distinguish) isn't immediately clear with if address entries will be discriminated/distinguished _using_ ports (i.e. considered) or ports will be discriminated _against_ (i.e. ignored).
## Breaking Changes
It's unclear if these backports result in serialization issues for older versions, as Dash Core technically supported address-port pairs since 0.12 and suppressed it on `mainnet` by setting zero-ing out the port ([source](19512988c6/src/addrman.cpp (L135-L138))), meaning even with port discrimination _disabled_, the serialization format should remain the same.
Regardless, following upstream backports, a new version has been introduced (v4) that marks the AddrMan format incompatible with older versions of Dash Core.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK a93fec6f2d
PastaPastaPasta:
utACK a93fec6f2d
Tree-SHA512: 49b35af3e4eb660249ce9a65d5a539205d852e9c728da22dc88779f6b3b15c13cf91522896a313bfe2a91889fedf3b6b2cebdea12cc2bbe865ec3b85b6a5dfa8
6777ab73a2 Merge bitcoin-core/gui#682: Don't directly delete abandoned txes from GUI (Hennadii Stepanov)
6e1a8c1fdc Merge bitcoin/bitcoin#26628: RPC: Reject RPC requests with same named parameter specified multiple times (MarcoFalke)
7c28b01c78 Merge bitcoin/bitcoin#26666: refactor: Deleted unreachable code in httpserver.cpp (MarcoFalke)
478fe51ead Merge bitcoin/bitcoin#26100: doc: clarify that NetPermissionFlags::Implicit is only about whitelists (MarcoFalke)
69b19cbfc0 Merge bitcoin/bitcoin#26546: test: remove unused class `NodePongAdd1` (fanquake)
245df942c4 Merge bitcoin/bitcoin#26380: Revert "test: check importing wallets when blocks are pruned throw an error" (MacroFake)
3db3bd0d31 Merge bitcoin/bitcoin#24269: test: add functional test for `-discover` (Andrew Chow)
c72ef299da Merge bitcoin/bitcoin#25896: wallet: Log when Wallet::SetMinVersion sets a different minversion (Andrew Chow)
12b438cb46 Merge bitcoin/bitcoin#25925: doc: add `{import,list}descriptors` to list of descriptor RPCs (Andrew Chow)
73d64c48a9 Merge bitcoin/bitcoin#25738: depends: use a patch instead of sed in libxcb (fanquake)
1fae0c2bbb Merge bitcoin/bitcoin#25333: test: Fix out-of-range port collisions (MacroFake)
b75089667c Merge bitcoin/bitcoin#25231: ci: Install documented packages for "Win64" CI task (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
Trivial backports
## How Has This Been Tested?
Built; ran tests locally
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 6777ab73a2
Tree-SHA512: 60d68c8d0fb9875d0b2421cad97b46a9d4deb79e03ca011d66cc0595ed68bb7ad207f2fd95973a3b7a0b9bca2c2f0deaf0e1116f0312a4d9d315f009228fe485
8c3ff7d52ae3314959e1e66da8718a3f0d30abaa test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca56382512df3084fce7353bf1e8b66cae61bc bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8cf27aa72ec2907342787e6fc9f94c50 rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e671e347e422d696d1cbdd9f82b2ce468 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)
Pull request description:
Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.
Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.
After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.
ACKs for top commit:
MarcoFalke:
review ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa 🗂
kristapsk:
ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa
stickies-v:
ACK 8c3ff7d52
Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
32f8fda7d6 merge bitcoin#24991: allow startup with -onlynet=onion -listenonion=1 (Kittywhiskers Van Gogh)
e67ed92d3d merge bitcoin#25173: add coverage for unknown network in -onlynet (Kittywhiskers Van Gogh)
77efd36112 merge bitcoin#24687: Check an invalid -i2psam will raise an init error (Kittywhiskers Van Gogh)
fb1416f7cb merge bitcoin#24205: improve network reachability test coverage and safety (Kittywhiskers Van Gogh)
7cb7479829 merge bitcoin#24663: add links to doc/cjdns.md (Kittywhiskers Van Gogh)
c736ebf566 merge bitcoin#24555: create initial doc/cjdns.md for CJDNS how-to documentation (Kittywhiskers Van Gogh)
554bd24186 partial bitcoin#24468: improve -onlynet help and related tor/i2p documentation (Kittywhiskers Van Gogh)
5436b6a82d merge bitcoin#24165: extend inbound eviction protection by network to CJDNS peers (Kittywhiskers Van Gogh)
d52724d039 merge bitcoin#22834: respect -onlynet= when making outbound connections (Kittywhiskers Van Gogh)
f9d1a9a00d merge bitcoin#23077: Full CJDNS support (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on https://github.com/dashpay/dash/pull/6034
* Depends on https://github.com/dashpay/dash/pull/6035
* If `-proxy=` is given together with `-noonion` then the provided proxy will not be set as a proxy for reaching the Tor network. So it will not be possible to open manual connections to the Tor network for example with the `addnode` RPC. To mimic the old behavior use `-proxy=` together with `-onlynet=` listing all relevant networks except `onion`.
* [bitcoin#24165](https://github.com/bitcoin/bitcoin/pull/24165) has been backported _before_ [bitcoin#23758](https://github.com/bitcoin/bitcoin/pull/23758) and to account for this, minor changes were made in `src/test/net_peer_eviction_tests.cpp` (using `nTimeConnected` instead of `m_connected`). When backporting [bitcoin#23758](https://github.com/bitcoin/bitcoin/pull/23758), these changes will have to be reversed as they won't be covered by the cherry-pick diff.
* CJDNS support has been labelled as being introduced in Dash Core 21.0, in line with the milestone designation of the PR. Should `develop` be used for a new minor/patch release, `doc/cjdns.md` will have to be modified to reflect the correct version number.
## Breaking changes
No expected protocol or consensus changes.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 32f8fda7d6
Tree-SHA512: e23b22ca5edbe4c4abeab0bc07780303e68e7c4cc46b7697300b0837c5acd3a98649b6b03bd07a23c827bd85f64210173027b0b0eea31872c031fa4ed04eeb0c
40bdc8a6e4dcf7faf90f63b0912c18d72436e37f test: remove unused class `NodePongAdd1` (Sebastian Falbesoner)
Pull request description:
This class was introduced in commit fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 ("net: Use mockable time for ping/pong, add tests"), but actually never used.
ACKs for top commit:
stickies-v:
ACK 40bdc8a6e
Tree-SHA512: b5a6552e4f2e0b7e368a071cc53b9a8e6f5d1950565a9fda8eb1971a01d8be0541d066842723ef44174fe8189925fa36f2defb6d7bf8d104abc77de410cc4c13
e1eadaa72d6831d1d0a53ba97c215dc4cdb64436 Revert "test: check importing wallets when blocks are pruned throw an error" (Aurèle Oulès)
Pull request description:
The test doesn't pass (not detected by the normal CI, because it is an extended test):
```
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/test_framework.py", line 133, in main
self.run_test()
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 480, in run_test
self.wallet_test()
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 361, in wallet_test
assert_raises_rpc_error(-4, "Importing wallets is disabled when blocks are pruned", self.nodes[2].importwallet, "abc")
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 145, in try_rpc
raise AssertionError(
AssertionError: Expected substring not found in error message:
substring: 'Importing wallets is disabled when blocks are pruned'
error message: 'Only legacy wallets are supported by this command'.
```
So revert it for now, which will be done anyway in https://github.com/bitcoin/bitcoin/pull/24865/commits. (This commit is taken from there)
ACKs for top commit:
andrewtoth:
ACK e1eadaa72d6831d1d0a53ba97c215dc4cdb64436
Tree-SHA512: 10f556ea1aa97dc9cb64f91055977eecb9534f658170aabb4909c3e85ca6c20588c7a021219356fab678e0e2bec999d347facd00054f07a9445ad393e6353b4c
bff05bd7456d3634b0c83539293a753db6d76376 test: add functional test for -discover (brunoerg)
Pull request description:
This PR adds a functional test for `-discover`. It tests different scenarios where `localaddresses` should be empty or may contain the addresses. Obs: `localaddresses` is not always accurate, so it's not possible to ensure (100%) it will contain any addresses.
515200298b/src/init.cpp (L449)
Obs: See #24258 - It adds test coverage for this field but for nodes with proxy.
ACKs for top commit:
mzumsande:
Code review ACK bff05bd7456d3634b0c83539293a753db6d76376
achow101:
ACK bff05bd7456d3634b0c83539293a753db6d76376
rajarshimaitra:
tACK bff05bd7456d3634b0c83539293a753db6d76376
Tree-SHA512: 8782497c146bce1ba86fda6146f3847465d7069f2cb6b84f2afc8f3b43efa813442bffe7447e9ce02adee304100b60365409bf0e5d875dfb880038442feec2a6
fa7a711a30b707cbdee4435dd0a956bffb7aaccb test: Fix out-of-range port collisions (MacroFake)
Pull request description:
Otherwise the test will fail if two tests running in parallel use the same port. See https://github.com/bitcoin/bitcoin/pull/25096#discussion_r892558783 and https://github.com/bitcoin/bitcoin/pull/25312
ACKs for top commit:
dergoegge:
ACK fa7a711a30b707cbdee4435dd0a956bffb7aaccb - This gets rid of some rather arbitrary choices for ports in some of our functional tests that can cause port collisions across test runs, resulting in intermittent failures.
Tree-SHA512: ac73da8a498230b992ab12e1ee3c4ff3d868cd63c00d2c71537d156cb7c8f8be8598ec574646b17c5a44ae3ac5bb54bf29d300f054a36cec6f6ce8054a0da0a4
76279c1a37 Merge bitcoin/bitcoin#25149: refactor: Add thread safety annotation to `BanMan::SweepBanned()` (MacroFake)
6269c6f1db Merge bitcoin/bitcoin#25053: Guard `#include <config/bitcoin-config.h>` (fanquake)
d50f0b016f Merge bitcoin/bitcoin#24977: rpc: Explain active and internal in listdescriptors (fanquake)
3c44399d55 Merge bitcoin/bitcoin#24856: lint: Converting lint-assertions.sh to lint-assertions.py (laanwj)
e9f5b4b735 Merge bitcoin/bitcoin#24213: refactor: use Span in random.* (laanwj)
7e0474ac1c Merge bitcoin/bitcoin#24632: add `(none)` in -getinfo `Warnings:` if no warning returned (laanwj)
57e9b56bad Merge bitcoin/bitcoin#24145: mempool: Clear vTxHashes when mapTx is cleared (laanwj)
fe56d9b994 Merge bitcoin/bitcoin#24698: test: -peerblockfilters without -blockfilterindex raises an error (MarcoFalke)
3cabce645e Merge bitcoin/bitcoin#24472: fuzz: execute each file in dir without fuzz engine (MarcoFalke)
f5116a7d31 Merge bitcoin-core/gui#549: refactor: use std::chrono for formatDurationStr() helper (Hennadii Stepanov)
3fa8158510 Merge bitcoin/bitcoin#22317: doc: Highlight DNS requests part in tor.md (Andrew Chow)
72b62edd5a Merge bitcoin/bitcoin#23834: wallettool: Check that the dumpfile checksum is the correct size (laanwj)
ee9b3cdb0e Merge bitcoin/bitcoin#23979: test: wait for rather than assert presence of file in startupnotify test (MarcoFalke)
2ec5940399 Merge bitcoin/bitcoin#23532: test: add functional test for -startupnotify (MarcoFalke)
5a31be9608 Merge bitcoin/bitcoin#23812: test: fix intermittent failures in p2p_timeouts.py (MarcoFalke)
10828f5b3d Merge bitcoin/bitcoin#23733: fuzz: Move ISO8601 to one place (MarcoFalke)
7f39b5af41 Merge bitcoin/bitcoin#23635: test: Bump shellcheck version to 0.8.0 (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Trivial batch of backports
## What was done?
trivial backports
## How Has This Been Tested?
Unit tests ran; waiting on CI
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: a3f97003e6441468951b827b2c3ea607740e5b9d36b96c2f93e45e7fb4088ecf4d0a2b7038de050ca0e7d61379c364969f4a8caff98ec1cc69016f4114e64c6a
f619f8ff80 merge bitcoin#20234: don't bind on 0.0.0.0 if binds are restricted to Tor (Kittywhiskers Van Gogh)
1698336fc8 merge bitcoin#22974: Improve performance of Good (Kittywhiskers Van Gogh)
29f4482e01 merge bitcoin#22911: Minor cleanups to asmap (Kittywhiskers Van Gogh)
5706edad5f merge bitcoin#22915: Remove confusing CAddrDB (Kittywhiskers Van Gogh)
3f69606827 merge bitcoin#22848: Expose BanMapToJson / BanMapFromJson (Kittywhiskers Van Gogh)
9065eed969 merge bitcoin#22791: Fix asmap/addrman initialization order bug (Kittywhiskers Van Gogh)
99b7812271 merge bitcoin#22849: Remove unused SERIALIZE_METHODS for CBanEntry (Kittywhiskers Van Gogh)
a30379c557 merge bitcoin#22740: Move serialization code to cpp (Kittywhiskers Van Gogh)
d4e79aa377 merge bitcoin#22697: Remove CAddrMan::Clear() function (Kittywhiskers Van Gogh)
77d8f6c918 merge bitcoin#22725: Move addrman ser/deser tests to addrman_tests.cpp (Kittywhiskers Van Gogh)
4ba3f49afc merge bitcoin#22627: De-duplicate Add() function (Kittywhiskers Van Gogh)
49af8185e0 merge bitcoin#20233: Make consistency checks a runtime option (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/6043
* [bitcoin#22915](https://github.com/bitcoin/bitcoin/pull/22915) is backported before [bitcoin#21850](https://github.com/bitcoin/bitcoin/pull/21850), which is the reason {`Dump`, `Read`}`PeerAddresses` takes in an `const ArgsManager&` but doesn't do anything with it.
* This will need to be accounted for when backporting [bitcoin#21850](https://github.com/bitcoin/bitcoin/pull/21850)
## Breaking Changes
None expected. No changes to serialization format.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK f619f8ff80
Tree-SHA512: 19599f0322f049b123790a45530ecb2ec74b1463b47e0102921b489bad1d5cb671a94900b3b35dbd7c1d80bc0680f1ed01e6fac675ac0be81621878388be0bcd
d6bc2322ed2e0674e027d39825fdadbb0db2c24a test: -peerblockfilters without -blockfilterindex raises an error (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
2a3e8fb359/src/init.cpp (L850)
Setting -peerblockfilters without -blockfilterindex should raise an error when initializing.
ACKs for top commit:
ccdle12:
Tested ACK d6bc2322ed
Tree-SHA512: e740c2ccde6bb1bb8381bb676a6d01bd5746cf9ce0c8dadd62067a6b9b380027bfe8b8cdeae9846a0ab18385f3dc5dff607fe5274cb55107d47470db00015fb2
ac617cc141fe05bea0dc5e8f9df3da43c0945842 wallettool: Check that the dumpfile checksum is the correct size (Andrew Chow)
Pull request description:
After parsing the checksum, make sure that it is the size that we expect it to be.
This issue was reported by Pedro Baptista.
ACKs for top commit:
laanwj:
Code review ACK ac617cc141fe05bea0dc5e8f9df3da43c0945842
Tree-SHA512: 8135b3fb1f4f6b6c91cfbac7d1d3421f1f6c664a742c92940f68eae857f92ce49d042cc3aa5c2df6ef182825271483d65efc7543ec7a8ff047fd7c08666c8899
96eb0093d07c58ad3b02c49a5a4385da37a8e337 test: wait rather than assert presence of file in startupnotify test (fanquake)
Pull request description:
Should fix#23967.
ACKs for top commit:
brunoerg:
crACK 96eb0093d07c58ad3b02c49a5a4385da37a8e337
kristapsk:
utACK 96eb0093d07c58ad3b02c49a5a4385da37a8e337
Tree-SHA512: 9107970e45c027cfc6c6cbfcfd5a7d9f5956259bbbb11f5b180c3947126e42e62c0f8ffd69cf7b39b51c9c5b4fedbb753839d59aebe876be68c1484bb6065819
126853214a490ee840e83ca17c717c40cfbe6837 test: add functional test for -startupnotify (Bruno Garcia)
Pull request description:
This PR adds a functional test for -startupnotify. It basically starts the node passing a command on -startupnotify to create a file on tmp and then, we check if the file has been successfully created.
ACKs for top commit:
theStack:
Tested ACK 126853214a490ee840e83ca17c717c40cfbe6837
kristapsk:
re-ACK 126853214a490ee840e83ca17c717c40cfbe6837
Tree-SHA512: 5bf3e46124ee5c9d609c9993e6465d5a71a8d2275dcf07c8ce0549f013f7f8863d483b46b7164152f566468a689371ccb87f01cf118c3c9cac5b2be673b61a5c
0a1b6fa5a18f3efb2ac3e28a23a4fd5e1cf9eaf0 test: fix intermittent timeouts in p2p_timeouts.py (Martin Zumsande)
Pull request description:
Fixes #23800 by making sure that all peers are connected (i.e. `m_connected` is set) before the mocktime is bumped.
We can't wait for verack here, but we can wait for a debug log entry ("Added connection peer=2") instead.
In the failed CI runs (e.g. https://cirrus-ci.com/task/5600553806856192?logs=ci#L7469) different peers were added at different mocktimes.
ACKs for top commit:
naumenkogs:
ACK 0a1b6fa5a18f3efb2ac3e28a23a4fd5e1cf9eaf0
theStack:
Concept and approach ACK 0a1b6fa5a18f3efb2ac3e28a23a4fd5e1cf9eaf0
Tree-SHA512: 1a3c8a9a79339d4adc6ecb1731eb0d0eadb2e5024ad3c6779b4696691f85d6c3304ef8689746d0332150a4cf04489ca4b2ff3eeb0bb76feec28c1e4bb9dbca19
c9095b738fd4257ae5bdbb2ae38d3e7f41f51b64 test: remove unnecessary assignment in bdb (Bruno Garcia)
Pull request description:
This PR removes the unnecessary assignment to page_info['entries'] on line 54 since there is another assignment for it in line 59.
I think a lint (#21096) would detect cases like this one.
ACKs for top commit:
achow101:
ACK c9095b738fd4257ae5bdbb2ae38d3e7f41f51b64
theStack:
Code Review ACK c9095b738fd4257ae5bdbb2ae38d3e7f41f51b64
Tree-SHA512: 23377077c015b04361fd416b41bf6806ad0bdd4d264be6760f0fd3bc88d694d2cd52cae250519925c5d3b3c70715772714c3863f8fa181a2eb4883204ccdbf9d
34c80473a8 Merge #19877: [test] clarify rpc_net & p2p_disconnect_ban functional tests (Wladimir J. van der Laan)
e42412924f Merge #19770: RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (MarcoFalke)
f96966b7ea Merge #20043: doc: Add 19501 release notes (fanquake)
6a164eaea9 Merge #19501: send* RPCs in the wallet returns the "fee reason" (MarcoFalke)
b6c8d852e3 Merge #19725: [RPC] Add connection type to getpeerinfo, improve logs (MarcoFalke)
f86263b180 Merge #18202: refactor: consolidate sendmany and sendtoaddress code (Samuel Dobson)
fab41fd3c5 partial Merge #18878: test: Add test for conflicted wallet tx notifications (Wladimir J. van der Laan)
db5bd34ee8 Merge #19202: log: remove deprecated `db` log category (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v21
## What was done?
- bitcoin/bitcoin#19202
- partial bitcoin/bitcoin#18878
- bitcoin/bitcoin#18202
- bitcoin/bitcoin#19725
- bitcoin/bitcoin#19501
- bitcoin/bitcoin#20043
- bitcoin/bitcoin#19770
- bitcoin/bitcoin#19877
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
- (RPC) The `getpeerinfo` RPC no longer returns the `addnode` field by default. This
field will be fully removed in the next major release. It can be accessed
with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However,
it is recommended to instead use the `connection_type` field (it will return
`manual` when addnode is true)
- (Settings) The `sendtoaddress` and `sendmany` RPCs accept an optional `verbose=True`
argument to also return the fee reason about the sent tx.
- (Settings) The `-debug=db` logging category, which was deprecated in v0.18 and replaced by
`-debug=walletdb` to distinguish it from `coindb`, has been removed.
- (RPC) To make RPC `sendtoaddress` more consistent with `sendmany` the following error
`sendtoaddress` codes were changed from `-4` to `-6`:
- Insufficient funds
- Fee estimation failed
- Transaction has too long of a mempool chain
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 34c80473a8
Tree-SHA512: 725a103e04c9c7d44a79da6f3f54e7745c7fb98ec906e7228ae16f7662d568e48c015c855902ff8485f2908f0f71815e769ca394cf6c3ca2e5fd920dd39cca74
47ff5098ad5ea2c20ea387f99940a7cde6c80789 [test] Clarify setup of node topology. (Amiti Uttarwar)
0672522aedd3760c30b8740c7e9487f00bf9dfeb [move-only, test]: Match test order with run order (Amiti Uttarwar)
Pull request description:
small improvements to clarify logic in the functional tests
1. have test logic in `rpc_net.py` match run order of the test
2. remove `connect_nodes` calls that are redundant with the automatic test setup executed by the test framework
Noticed when I was trying to debug a test for #19725. Small changes but imo very helpful, because they initially confused me.
ACKs for top commit:
laanwj:
ACK 47ff5098ad5ea2c20ea387f99940a7cde6c80789
Tree-SHA512: 2843da2c0b4f06b2600b3adb97900a62be7bb2228770abd67d86f2a65c58079af22c7c20957474a98c17da85f40a958a6f05cb8198aa0c56a58adc1c31100492
5b57dc5458800e56b4dddfeb32a1813804a62b0f RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg (Luke Dashjr)
d681a28219d3876a2b6e3cd2fb0d92963674903e RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (Luke Dashjr)
Pull request description:
If we were going to continue support for "whitelisted", we should have probably made it true if any permission flag was set, rather than only if "default permissions" were used.
This corrects the description, and deprecates it.
ACKs for top commit:
laanwj:
ACK 5b57dc5458800e56b4dddfeb32a1813804a62b0f
Tree-SHA512: a2e2137f8be8110357c1b2fef2c923fa8c7c4a49b0b2b3a2d78aedf12f8ed5cc7e140018a21b37e6ec7770ed4007542aeef7ad4558973901b107e8e0f81d6003
fa710a6d67b2de64bde90def77c70d0a052f9030 doc: Add 19501 release notes (MarcoFalke)
faf60dee34ae3dbe8e103a2c1b0679f13df6a921 doc: Remove double-whitespace from help string, other whitespace fixups (MarcoFalke)
Pull request description:
Adds release notes and fixes up some whitespace nits for the touched RPCs
ACKs for top commit:
fanquake:
ACK fa710a6d67b2de64bde90def77c70d0a052f9030
laanwj:
Code review ACK fa710a6d67b2de64bde90def77c70d0a052f9030
Tree-SHA512: b84a96386a9a8ed69f464c7dffdd600cf9a8b33a06120798b141b300991baed369ab91ae48df6446e89e1d62534ccd8ae721454e7a19b48900b317e9192afc47
69cf5d4eeb73f7d685e915fc17af64634d88a4a2 [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e20d56acf7246008b7832efde68ab21 [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb73f7d685e915fc17af64634d88a4a2
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar)
50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
sipa:
utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
promag:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
08fc6f6cfc3b06fd170452a766696d7b833113fa [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost)
Pull request description:
I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination.
Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this.
Salvaged from #18201.
ACKs for top commit:
fjahr:
Code review ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
jonatack:
ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
meshcollider:
Code review & functional test run ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909
bde72a41fe merge bitcoin#23324: print peer counts for all reachable networks in -netinfo (Kittywhiskers Van Gogh)
4b245441a0 merge bitcoin#22959: Display all proxies in -getinfo (Kittywhiskers Van Gogh)
30b0fcf4a6 merge bitcoin#22544: drop torv2; torv3 becomes onion per GetNetworkName() (Kittywhiskers Van Gogh)
b6ca36edda merge bitcoin#22547: Add progress bar for -getinfo (Kittywhiskers Van Gogh)
1f89bfd176 merge bitcoin#21832: Implement human readable -getinfo (Kittywhiskers Van Gogh)
2200b78a15 merge bitcoin#20877: user help and argument parsing improvements (Kittywhiskers Van Gogh)
bd934c71eb partial bitcoin#20764: cli -netinfo peer connections dashboard updates (Kittywhiskers Van Gogh)
b2d865633f merge bitcoin#21261: update inbound eviction protection for multiple networks, add I2P peers (Kittywhiskers Van Gogh)
0b16b50fcb cli: fix loop counter comparison in `ProcessReply` (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/6035
* Dependency for https://github.com/dashpay/dash/pull/6031
* In [dash#5904](https://github.com/dashpay/dash/pull/5904) ([bitcoin#21595](https://github.com/bitcoin/bitcoin/pull/21595)), one of the loops in `ProcessReply` is supposed to iterate `rows.size()` times (which at the time was hardcoded to `3`), the backport erroneously set the value to `m_networks.size()` (which also evaluated to `3`) as part of increasing `m_networks.size()` usage.
As this pull request includes [bitcoin#23324](https://github.com/bitcoin/bitcoin/pull/23324), which changes it over to `rows.size()`, the above has been corrected in a separate commit for documentation purposes.
* `-addrinfo` output
* `-getinfo` output (diamonds are due to rendering limitations of my terminal and are not indicative of the symbols used)
* `-netinfo` output
## Breaking Changes
* CLI `-addrinfo` now returns a single field for the number of `onion` addresses known to the node instead of separate `torv2` and `torv3` fields, as support for TorV2 addresses was removed from Dash Core in 18.0.
* `-getinfo` has been updated to return data in a user-friendly format that also reduces vertical space.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK bde72a41fe
Tree-SHA512: 921cb45b7e243a321a32c835eb23d5ba8df610ff234a548a9051436a2c21845ce70097fb9a9bb812b77b04373f9f0a9f90264168d97b08da1890be06bfd9f99c
5730a43703f7e5a5ca26245ba3b55fbdd027d0b6 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f93979b274a37de013502b05d25fad8 net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d9072b7d5a36a6491784bdeb9247e91fb0 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450dde6a54bd785504c923adfb45c7060 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
naumenkogs:
ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
Backport notice:
we don't have bumpfee feature, so, only some part of code is backported
f963a680515eda66429b3d1537a7baf281ab9283 test: Add test for conflicted wallet tx notifications (Russell Yanofsky)
Pull request description:
Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions
https://github.com/bitcoin/bitcoin/pull/9240 - accidental break
https://github.com/bitcoin/bitcoin/issues/9479 - bug report
https://github.com/bitcoin/bitcoin/pull/9371 - fix
https://github.com/bitcoin/bitcoin/pull/16624 - accidental break
https://github.com/bitcoin/bitcoin/issues/18325 - bug report
https://github.com/bitcoin/bitcoin/pull/18600 - potential fix
ACKs for top commit:
laanwj:
ACK f963a680515eda66429b3d1537a7baf281ab9283
jonatack:
re-ACK f963a680515eda66429b3d1537a7baf281ab9283
MarcoFalke:
ACK f963a680515eda66429b3d1537a7baf281ab9283
Tree-SHA512: d3a7952a2d3dc2ff0800ef857575ea4ef9759c0917d58a7fc91e2db0ca3cc3baf0dd0cf9af61683f691e5fefb11afe8120bb5810c7037ed9ecedee385dd4aa07
fixup dashify of feature_notifications
5fe8100ff36fed6d50c2a25b028f57b25af3504c Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. (lsilva01)
ae23faba6fc5cabc896f1175456d1018576f912d Add a new RPC command: restorewallet (lsilva01)
Pull request description:
As far as I know, there is no command to restore the wallet from a backup file.
The only way to do this is to replace the `wallet.dat` of a newly created wallet with the backup file, which is hardly an intuitive way.
This PR implements the `restorewallet` RPC command which restores the wallet from the backup file.
To test:
First create a backup file:
`$ bitcoin-cli -rpcwallet="wallet-01" backupwallet /home/Backups/wallet-01.bak`
Then restore it in another wallet:
`$ bitcoin-cli restorewallet "restored-wallet-01" /home/Backups/wallet-01.bak`
ACKs for top commit:
achow101:
re-ACK 5fe8100ff36fed6d50c2a25b028f57b25af3504c
prayank23:
tACK 5fe8100ff3
meshcollider:
utACK 5fe8100ff36fed6d50c2a25b028f57b25af3504c
Tree-SHA512: 9639df4d8ad32f255f5b868320dc69878bd9aceb3b471b49dfad500b67681e2d354292b5410982fbf18e25a44ed0c06fd4a0dd010e82807c2e00ff32e84047a1
09205b33aa74e385caa2803aa6febc18ad1efa32 net: Clarify message header validation errors (W. J. van der Laan)
955eee76803c098978cf0bbc7f1f6d3c230544e2 net: Sanitize message type for logging (W. J. van der Laan)
Pull request description:
- Use `SanitizeString` when logging message errors to make sure that the message type is sanitized. I have checked all logging in `net.cpp`.
- For the `MESSAGESTART` error don't inspect and log header details at all: receiving invalid start bytes makes it likely that the packet isn't even formatted as valid P2P message. Logging the four unexpected start bytes (as hex) should be enough.
- Update `p2p_invalid_messages.py` test to check this.
- Improve error messages in a second commit.
Issue reported by gmaxwell.
ACKs for top commit:
MarcoFalke:
re-ACK 09205b33aa74e385caa2803aa6febc18ad1efa32 only change is log message fixup 🔂
practicalswift:
re-ACK 09205b33aa74e385caa2803aa6febc18ad1efa32
Tree-SHA512: 8fe5326af135cfcf39ea953d9074a8c966b9b85a810b06a2c45b8a745cf115de4f321e72fc769709d6bbecfc5953aab83176db6735b04c0bc6796f59272cadce
754e802274e9373ad7e1dccb710acf74ded6e7fb test: check rejected future block later accepted (Luke Dashjr)
Pull request description:
(Luke) was unsure if the code sufficiently avoided caching a
time-too-new rejection, so wrote this test to check it. It looks like
despite only exempting BLOCK_MUTATED, it is still okay because header
failures never cache block invalidity. This test will help ensure that
if this ever changes, BLOCK_TIME_FUTURE gets excluded at the same time.
This PR re-opens https://github.com/bitcoin/bitcoin/pull/17872 which went stale and addresses the nits raised by reviewers there.
ACKs for top commit:
MarcoFalke:
review ACK 754e802274e9373ad7e1dccb710acf74ded6e7fb
Tree-SHA512: a2bbc8fffb523cf2831e1ecb05f20868e30106a38cc2e369e4973fa549cca06675a668df16f76c49cc4ce3a22925404255e5c53c4232d63ba1b9fca878509aa0
ef5e9304cd407adab1563f24215da1b582274c20 test: update logging and docstring in rpc_blockchain.py (Jon Atack)
d548dc71e4849f638fccaea6be86ac4fa5304f01 test: replace magic values by constants in rpc_blockchain.py (Jon Atack)
78c361086fc0bf27612e8142bd33e05e37a36af6 test: assert on mediantime in getblockheader and getblockchaininfo (Jon Atack)
0a9129c588ab016eb0453b40a0cae918ca4aa6a2 test: assert on the value of getblockchaininfo#time (Jon Atack)
Pull request description:
Follow-up to #22407 improving test coverage per https://github.com/bitcoin/bitcoin/pull/22407#pullrequestreview-702077013.
ACKs for top commit:
tryphe:
untested ACK ef5e9304cd407adab1563f24215da1b582274c20
Tree-SHA512: f746d56f430331bc6a2ea7ecd27b21b06275927966aacf1f1127d8d5fdfd930583cabe72e23df3adb2e005da904fc05dc573b8e5eaa2f86e0e193b89a17a5734
a006d7d73019b8cf4d68626c019c3d69729dda69 test: add logging to wallet_listtransactions (Sebastian Falbesoner)
47915b118720c6e2b2ec9f599f25848041b42b99 test: remove unneeded/redundant code in wallet_listtransactions (Sebastian Falbesoner)
fb6c6a7938cb7c4808ad88d23bfc2b7408407b12 test: speedup wallet_listtransactions by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
This PR improves the test `wallet_listtransactions.py` in three ways:
* speeds up runtime by a factor of 2-3x by using the good ol' immediate tx relay trick (`-whitelist=noban@127.0.0.1`)
* removes unneeded/redundant code
* adds log messages, mostly by turning comments into `self.log.info(...)` calls
ACKs for top commit:
jonatack:
ACK a006d7d73019b8cf4d68626c019c3d69729dda69
kristapsk:
ACK a006d7d73019b8cf4d68626c019c3d69729dda69
Tree-SHA512: a91a19f5ebc4d05f0b96c5419683c4c57ac0ef44b64eeb8dd550bd72296fd3a2857a3ba83f755fe4b0b3bd06439973f226070b5d0ce2dee58344dae78cb50290
ba7e17e073f833eccd4c7c111ae9058c3f123371 rpc, test: document {previous,next}blockhash as optional (Sebastian Falbesoner)
Pull request description:
This PR updates the result help of the following RPCs w.r.t. the `previousblockhash` and `nextblockhash` fields:
- getblockheader
- getblock
Also adds trivial tests on genesis block (should not contain "previousblockhash") and best block (should not contain "nextblockhash").
Top commit has no ACKs.
Tree-SHA512: ef42c5c773fc436e1b4a67be14e2532e800e1e30e45e54a57431c6abb714d2c069c70d40ea4012d549293b823a1973b3f569484b3273679683b28ed40abf46bb
a33dcb3283 fix: CheckWalletOwnsScript/CheckWalletOwnsKey to use wallet instead of SPK (Konstantin Akimov)
b2ede8bfee feat: update list of tests that still doesn't support descriptor wallets (Konstantin Akimov)
838d06f2fd feat: enable descriptor wallets for more tests (Konstantin Akimov)
5ab108c982 feat: implementation of RPC 'protx register' for descriptor wallets (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Many rpc such as `protx register` uses forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as
```
test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
```
for all functional tests that uses Masternodes/evo nodes.
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## What was done?
Some direct usages of LegacyScriptPubKeyMan refactored to use CWallet's functionality.
There are still 4 functional tests that doesn't work for descriptor wallets:
- feature_dip3_deterministicmns.py (no rpc `protx updateregistar`)
- feature_governance.py: no rpc for `governance votemany` and `governance votealias`
- interface_zmq_dash.py (see governance)
That's part I of changes, other changes are not PR-ready yet, WIP.
## How Has This Been Tested?
Firstly, the flag `--legacy-wallets` are removed for many functional tests.
Secondly, the flag `--descriptors` is inverted in default value:
```
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 585a6a74d6..9ad5fd1daa 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
if self.options.descriptors is None:
# Prefer BDB unless it isn't available
- if self.is_bdb_compiled():
- self.options.descriptors = False
- elif self.is_sqlite_compiled():
+ if self.is_sqlite_compiled():
self.options.descriptors = True
+ elif self.is_bdb_compiled():
+ self.options.descriptors = False
else:
# If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter
# It still needs to exist and be None in order for tests to work however.
```
## Breaking Changes
N/A, descriptor wallets have not been publicly released yet
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK a33dcb3283
Tree-SHA512: 24e22ac91e30a3804d1ac9af864eb9d073208bbb6d1f3326c7f0438f3ccce2b553aa46450989e48cb5c6e0d838ff1c88c6522f195e7aa2bd89342710f3ecef77
e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886b60644d103a5eb310691c003a39328 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2f2e139fbc63e2f37fb33ea4efae088 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a83f3e94bcb78b6aa463c5570c1e737e Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8ba69e2cc82c617cdf406ab79df58825 wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e5434da90d2cf680f23e8c7b7164c9f945 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b834077f102724415e0fada6ee8b2def2 descriptors: Cache last hardened xpub (Andrew Chow)
cacc3910989c4f3d7afa530dbab042461426abce Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75cd03c8f0a8cfadb47e0fbcabe3c5e59 Refactor Cache merging and writing (Andrew Chow)
976b53b085d681645fd3a008fe382de85647e29f Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
3eb6f8b2e61c24a22ea9396d86672307845f35eb wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb56bf5d455154b0498b1f58f77d20ed wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e357159c7154f69f28cb5587c5ca20d6594 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce88696a3216fc38b7d393906b733e8b1 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788cb0862aafbb116fd37936cbed6a431 wallet: refactor GetClosestWalletFeature() (Jon Atack)
Pull request description:
This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in https://github.com/bitcoin/bitcoin/pull/18836#discussion_r519328607.
This PR fixes 4 upgradewallet issues:
- this bug: https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
- it returns nothing in the absence of an RPC error, which isn't reassuring for users
- it returns the same thing both in the case of a successful upgrade and when no upgrade took place
- the error message object is currently dead code
This PR fixes the above and provides:
...user feedback to not silently return without upgrading
```
{
"wallet_name": "disable private keys",
"previous_version": 169900,
"current_version": 169900,
"result": "Already at latest version. Wallet version unchanged."
}
```
...better feedback after successfully upgrading
```
{
"wallet_name": "watch-only",
"previous_version": 159900,
"current_version": 169900,
"result": "Wallet upgraded successfully from version 159900 to version 169900."
}
```
...helpful error responses
```
{
"wallet_name": "blank",
"previous_version": 169900,
"current_version": 169900,
"error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
}
{
"wallet_name": "blank",
"previous_version": 130000,
"current_version": 130000,
"error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
}
```
updated help:
```
upgradewallet ( version )
Upgrade the wallet. Upgrades to the latest version if no version number is specified.
New keys may be generated and a new wallet backup will need to be made.
Arguments:
1. version (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"previous_version" : n, (numeric) Version of wallet before this operation
"current_version" : n, (numeric) Version of wallet after this operation
"result" : "str", (string, optional) Description of result, if no error
"error" : "str" (string, optional) Error message (if there is one)
}
```
ACKs for top commit:
achow101:
ACK 3eb6f8b
MarcoFalke:
review ACK 3eb6f8b2e61c24a22ea9396d86672307845f35eb 🛡
Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
5f9c0b6360215636cfa62a70d3a70f1feb3977ab wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08215feba53ead27096ac7fda34acb3c test: Remove unused wallet.dat (MarcoFalke)
bf7635963c03203e7189ddaa56c6b086a0108cbf tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9decc3e855ee4b0bbf9e61121c8e9904e5 test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc434854f881330771a93a1280ac67b1d3549 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19be65b0dd23df1df571c71428c2bc32 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c995e832e643f605d35a7aa112837e6 wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc6258c258e9f4411c50630ec4a552341b wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f34dedf75b063b962845fa8eca604514 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842df489f1b8d68e67a234788966218184 wallet: Add utility method for CanSupportFeature (Andrew Chow)
Pull request description:
This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases.
The `nWalletMaxVersion` member variable has been removed as it made `CanSupportFeature` unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old `-upgradewallet` option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the `upgradewallet` RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. `nWalletMaxVersion` was only relevant for upgrades to `FEATURE_WALLETCRYPT` and `FEATURE_COMPRPUBKEY` both of which are incredibly old features. So for such wallets, the behavior of `upgradewallet` will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that `FEATURE_WALLETCRYPT` indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated.
`CanSupportFeature` would previously indicate whether we could upgrade to `nWalletMaxVersion` not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into `ScriptPubKeyMan::Upgrade` the version we are upgrading to and checking against that. By removing `nWalletMaxVersion` we also fix a bug where you could upgrade to HD chain split without the pre-split keypool.
`nWalletMaxVersion` was also the version that was being reported by `getwalletinfo` which meant that the version reported was not always consistent across restarts as it depended on whether `upgradewallet` was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to `upgradewallet`, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900.
Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR.
Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to `upgradewallet`, `UpgradeKeyMetadata` is now being tested too.
Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function `sha256sum_file` has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new `bdb.py` module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file `dbinc/db_page.h` describe it. This module just dumps all of the fields into a dict.
ACKs for top commit:
MarcoFalke:
approach ACK 5f9c0b6360
laanwj:
Code review ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab
jonatack:
ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the `wallet_upgradewallet.py` test runs green both before and after running `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`
Tree-SHA512: 7c4ebf420850d596a586cb6dd7f2ef39c6477847d12d105fcd362abb07f2a8aa4f7afc5bfd36cbc8b8c72fcdd1de8d2d3f16ad8e8ba736b6f4f31f133fe5feba
The key difference between bitcoin's and dash's implementation that sethdseed
does not update existing seed for wallet. Seed can be set only once.
It behave similarly to `upgradetohd` rpc, but since v20.1 all wallets are HD and
the name `upgradetohd` is not relevant more.
That are:
- feature_dip3_deterministicmns.py
- interface_zmq_dash.py
- feature_governance.py
- wallet_upgradetohd.py (as expected to be implemented for legacy-only wallets)
- p2p_timeouts.py (why? can not understand it)
This partially reverts commit b20f812674.
ccd976dd3dbb8f991dc1203ada2043f1736be5a4 test: use 327 fewer blocks in feature_nulldummy (Jon Atack)
68c280f19732fb96bc29113ce9c8007d0101868c test, refactor: abstract the feature_nulldummy blockheight values (Jon Atack)
Pull request description:
The resolved timeout issue seen in the CI can be reproduced locally by running `test/functional/feature_nulldummy.py --valgrind --loglevel=debug`
Speeds up the normal test runtime for me from 3.8 to 2.2 seconds (debug build). Thanks to Marco Falke for the approach suggestion.
ACKs for top commit:
AnthonyRonning:
reACK ccd976dd3dbb8f991dc1203ada2043f1736be5a4 - ran a few times with the rest of the tests and still passing for me with just the fewer block change.
MarcoFalke:
review ACK ccd976dd3dbb8f991dc1203ada2043f1736be5a4 🏝
Tree-SHA512: 38339dca4276d1972e3a5a5ee436da64e9e58fd3b50b186e34b07ade9523ac4c03f6c3869c5f2a59c23b43c44f87e712f8297a01a8d83202049c081e3eeb4445
ffe33dfbd4c3b11e3475b022b6c1dd077613de79 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2c2ce5fea84cf8681013f85a444e7ea versionbits: simplify state transitions (Anthony Towns)
55ac5f568a3b73d6f1ef4654617fb76e8bcbccdf versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48040dc7eae46bc7941db48d98a669fd fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c1702c8ae259610fe55050ba212e21e tests: test versionbits delayed activation (Anthony Towns)
73d4a706393e6dbd6b6d6b6428f8d3233ac0a2d8 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa205eee5c3b99343988adcb8d320460 tests: clean up versionbits test (Anthony Towns)
593274445004506c921d5d851361aefb3434d744 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a4760c0c0f784029849cb5d21ee088abb tests: pull ComputeBlockVersion test into its own function (Anthony Towns)
Pull request description:
BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html
Edge cases are tested by fuzzing added in #21380.
ACKs for top commit:
instagibbs:
tACK ffe33dfbd4
jnewbery:
utACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
MarcoFalke:
review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈
achow101:
re-ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
gmaxwell:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
benthecarman:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
Sjors:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
jonatack:
Initial approach ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
ariard:
Code Review ACK ffe33df
Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672 validation: remove nchaintx from assumeutxo metadata (James O'Beirne)
931684b24a89aba884cb18c13fa67ccca339ee8c validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne)
Pull request description:
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.
Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
See: https://github.com/bitcoin/bitcoin/pull/19806#discussion_r612165410
ACKs for top commit:
Sjors:
utACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672
ryanofsky:
Code review ACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.
Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
10a006e626 test: disable ipv6 tests for now (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Our CI nodes on Amazon have issues running tests using ipv6.
## What was done?
Pretend we don't have ipv6 when we run tests for now
## How Has This Been Tested?
See CI results for this PR.
## Breaking Changes
n/a but we'll have ipv6 not being tested for some time.
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
kwvg:
ACK 10a006e626
PastaPastaPasta:
utACK 10a006e
Tree-SHA512: 75da180b916a5a99b1363f152494cbdd1031c7daf2d5eb370ca74547dee694ad04db3e9c677f0f738b9dbb15760d685a6b43b032dde642a4c1d49fff671e2aac
68ace23fa3bc01baa734ddf2c0963acae1c75ed1 test: convert docs into type annotations in test_framework/test_node.py (fanquake)
8bfcba36db974326d258c610456bd55cf5818b1e test: convert docs into type annotations in test_framework/wallet.py (fanquake)
b043ca8e8b65199061ebe4bbed2200504dfc6ce9 test: convert docs into type annotations in test_framework/util.py (fanquake)
Pull request description:
Rather than having function types exist as documentation, make them type annotations, which enables more `mypy` checking.
ACKs for top commit:
instagibbs:
utACK 68ace23fa3
Tree-SHA512: b705f26b48baabde07b9b2c0a8d547b4dcca291b16eaf5ac70462bb3a1f9e9c2783d93a9d4290889d0cbb3f7db3671446754411a1f498b265d336f6ff33478df
7a681d61b0d98a310fbb1b8e095ab8fbc5d5741c Add sync_blocks in wallet_orphanedreward.py. (Daniel Kraft)
Pull request description:
Add an explicit `sync_blocks` call in `wallet_orphanedreward.py`, which was missing and could lead to intermittent failures of the test due to race conditions.
This will presumably fix#22181.
ACKs for top commit:
MarcoFalke:
review ACK 7a681d61b0d98a310fbb1b8e095ab8fbc5d5741c
Tree-SHA512: bb226c31bf3f2e7c52beb829d7b67496e5b38781245db5f9184e3f28c93ac3aa4d21fcf5bf3055e79d384cfd0ed916e79dccb3d77486e86fe1fedb5e35f894ad
44dab423eb88dbf854d22f2991e79c828ffac0f2 qa: Test default include_mempool value of gettxout (João Barbosa)
Pull request description:
With the following diff the functional test would pass. Fix by testing the default value.
```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1142,7 +1142,7 @@ static RPCHelpMan gettxout()
uint256 hash(ParseHashV(request.params[0], "txid"));
int n = request.params[1].get_int();
COutPoint out(hash, n);
- bool fMempool = true;
+ bool fMempool = false;
if (!request.params[2].isNull())
fMempool = request.params[2].get_bool();
```
ACKs for top commit:
MarcoFalke:
cr ACK 44dab423eb88dbf854d22f2991e79c828ffac0f2
Tree-SHA512: 14db21b29d6b2c01d1d1278e18a0cf35d6ae566e33e45515d1fe2983dda94ad1ff6065c217601d283f9515cae39b57e981b62ac71ec2002de5359bd8a9e3efa9
ceefab5226 fix: feature_backwards compatible works now with as expected if no bdb compiled (Konstantin Akimov)
b20f812674 fix: follow-up fixes for functional tests used protx (Konstantin Akimov)
655146d5e7 Merge #21302: wallet: createwallet examples for descriptor wallets (W. J. van der Laan)
99a8b60393 Merge #21063: wallet, rpc: update listdescriptors response format (fanquake)
6ee2c7cc59 Merge #21277: wallet: listdescriptors uses normalized descriptor form (Wladimir J. van der Laan)
8bacdbf71f Merge #19136: wallet: add parent_desc to getaddressinfo (Samuel Dobson)
f567de007a chore: release notes for 5965 with wallet tool improvements (Konstantin Akimov)
0daf360edf chore: add TODO to implement mnemonic for descriptor wallets (Konstantin Akimov)
5016294307 chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test" (Konstantin Akimov)
ef7ce87c1b fix: remove workarounds introduced due to missing bitcoin#20267 (bdb is not compiled) (Konstantin Akimov)
06b2d85bb4 partial Merge #20267: Disable and fix tests for when BDB is not compiled (Wladimir J. van der Laan)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/59
## Extra notes
This commit `chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test"` is not directly connected to descriptor wallets, but added to this PR due to conflicts with 20267
## What was done?
It steadily improves support of descriptor wallets in Dash core.
Done backports and related fixes:
- partial bitcoin/bitcoin#20267
- bitcoin/bitcoin#19136
- bitcoin/bitcoin#21277
- bitcoin/bitcoin#21063
- bitcoin/bitcoin#21302
Beside backports and related fixes, this PR includes release notes for previous batch of backports for descriptor wallets support #5965
## How Has This Been Tested?
Run unit functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
Top commit has no ACKs.
Tree-SHA512: f4b2033f8c4fa1d0f72cfc31378909703b3ae8f44748989ff00c3e71311ac80ac37837137133c7e4a166823a941ed7df10efa09c89f5b213f3c8ede7d3d6e8f4
1fedf470cd test: add type annotation for `ADDRS` in `p2p_addrv2_relay` (Kittywhiskers Van Gogh)
022b76f20b merge bitcoin#23218: Use mocktime for ping timeout (Kittywhiskers Van Gogh)
45d9e58023 merge bitcoin#22960: Set peertimeout in write_config (Kittywhiskers Van Gogh)
06e909b737 merge bitcoin#22604: address rate-limiting follow-ups (Kittywhiskers Van Gogh)
60b3e08ed1 merge bitcoin#22616: address relay fixups (Kittywhiskers Van Gogh)
8b8fbc5226 merge bitcoin#22618: Small follow-ups to 21528 (Kittywhiskers Van Gogh)
18fe765988 merge bitcoin#21528: Reduce addr blackholes (Kittywhiskers Van Gogh)
c1874c6615 net_processing: gate `m_tx_relay` access behind `!IsBlockOnlyConn()` (Kittywhiskers Van Gogh)
602d13d2a2 merge bitcoin#22387: Rate limit the processing of rumoured addresses (Kittywhiskers Van Gogh)
fe66202c05 merge bitcoin#22211: relay I2P addresses even if not reachable (by us) (Kittywhiskers Van Gogh)
7e08db55fe merge bitcoin#22306: Improvements to p2p_addr_relay.py (Kittywhiskers Van Gogh)
ff3497c18b merge bitcoin#21843: enable GetAddr, GetAddresses, and getnodeaddresses by network (Kittywhiskers Van Gogh)
51edeb082c merge bitcoin#21594: add network field to getnodeaddresses (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/5982
* Population of `ADDRS` in `p2p_addr`(`v2`)`_relay` in Dash is done in the test object ([source](0a62b9f985/test/functional/p2p_addrv2_relay.py (L42-L49))) as opposed to upstream, where it is done in the global state ([source](d930c7f5b0/test/functional/p2p_addrv2_relay.py (L23-L35))). This is because Dash specifically relies on `self.mocktime` instead of Bitcoin, which will work with simply sampling current time (`time.time()`).
* [bitcoin#22211](https://github.com/bitcoin/bitcoin/pull/22211) adds changes ([source](https://github.com/bitcoin/bitcoin/pull/22211/files#diff-d3d7b1bb23f25a96c9c7444a79159ad1799895565f99efebf1618e41e886bd53R44-R46)) that add usage of `ADDRS` outside the test object. That, alongside with other considerations, resulted in [dash#5967](https://github.com/dashpay/dash/pull/5967) and a discussion ([source](https://github.com/dashpay/dash/pull/5967/files#r1548101561))
* Eventually, following the footsteps of [dash#5967](https://github.com/dashpay/dash/pull/5967), `ADDRS` was defined outside but setup within the test object. This worked just fine ([build](https://gitlab.com/dashpay/dash/-/jobs/6594036014)) but displeased the linter ([build](https://gitlab.com/dashpay/dash/-/jobs/6594035886)) because `ADDRS` type could not be implicitly determined solely on usage in the global scope.
* An attempt to correct this was done by realignment with upstream ([commit](262d00682c)), which pleased the linter ([build](https://gitlab.com/dashpay/dash/-/jobs/6597322521)) but broken the test ([build](https://gitlab.com/dashpay/dash/-/jobs/6597322548)) for the reasons as mentioned above.
* Therefore, to keep the linter happy, `ADDRS` has been annotated as a `List[CAddress]` (which involved importing `List` but that's fine) ([commit](cb6d36df7d))
* Working on [bitcoin#21528](https://github.com/bitcoin/bitcoin/pull/21528) proved challenging due to differences in Dash's and Bitcoin's approach to relaying and the workarounds used to accommodate for that.
* Bitcoin conditionally initializes `m_tx_relay` ([source](3f7250b328/src/net.cpp (L2989-L2991))) and can always check if transaction relaying is permitted by checking if it's initialized ([source](3f7250b328/src/net_processing.cpp (L1820-L1826))).
* Dash unconditionally initializes it ([source](0a62b9f985/src/net.h (L605-L607))). Earlier, Dash used to check if it's _appropriate_ to relay transactions by checking if it can relay addresses ([source](dc6f52ac99/src/net_processing.cpp (L2134-L2140))), which at the time, simply meant, it wasn't a block-only connection ([source](dc6f52ac99/src/net.h (L568-L572))).
* This mutual exclusivity no longer held true in [dash#5964](https://github.com/dashpay/dash/pull/5964) and therefore, some transaction relay decisions were bound to **not** being a block-only connection ([commit](26c39f5b92)) but some were left behind, adopting `RelayAddrsWithPeer()` ([source](0a62b9f985/src/net_processing.cpp (L2215-L2221))), which, to be noted, is determined by the initialization status of `Peer::m_addr_known` ([source](0a62b9f985/src/net_processing.cpp (L839-L842))), which, so far, was pegged to **not** block-relay connection status ([source](0a62b9f985/src/net_processing.cpp (L1319))).
* [bitcoin#21528](https://github.com/bitcoin/bitcoin/pull/21528) got rid of `RelayAddrsWithPeer()` and replaced it with `Peer::m_addr_relay_enabled` ([source](3f7250b328/src/net_processing.cpp (L237-L251))), which is setup using `Peer::SetupAddressRelay()` ([source](3f7250b328/src/net_processing.cpp (L637-L643))). This means, rather than defining the address relay status during construction, it is setup during the first address-related message (i.e. `ADDR`, `ADDRV2`, `GETADDR`) ([source](3f7250b328/src/net_processing.cpp (L227-L236))).
* Meaning, until the first addr-related message happens, the state is has not been determined and defaults to `false`. Because some `m_tx_relay` usage still piggybacked on addr-relay permission to determine tx-relay, if a transaction message is processed before an address message is processed, there will be a false-negative condition.
The transaction relay logic won't run since it's expecting that if transactions can be relayed, so can addresses and checks for address relaying but believes that it cannot do address relaying, borrowing that state for transaction relaying, despite address relaying permissions actually being indeterminate since it hasn't had a chance to validate its eligibility.
* There were two approaches, run `SetupAddressRelay()` as early in the connection as possible to substitute for the "determine at construction" behaviour and change no other conditional statements... and break address-related tests _or_ move the remaining conditional transaction relay logic to use **not** block-only connection checks instead.
* We've gone with the latter, resulting in some changes where the condition only changes form but is the same (`RelayAddrsWithPeer()` > `Peer::m_addr_relay_enabled`) ([source](109c5a9383 (diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L2131-L2134))) but other changes where the condition itself has been changed (`RelayAddrsWithPeer()` > `!CNode::IsBlockOnlyConn()`) ([source](109c5a9383 (diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2256-R2259)))
* This does mean that in [dash#5982](https://github.com/dashpay/dash/pull/5982), `Peer::m_block_relay_only` is introduced to be the counterpart to `Peer::m_addr_relay_enabled` ([source](45b48dae0a/src/net_processing.cpp (L321-L322))) to account for some `CConnman` logic being moved into `PeerManager` ([source](45b48dae0a/src/net_processing.cpp (L2186-L2195))), which, in a way, reverts [dash#5339](https://github.com/dashpay/dash/pull/5339) but also, doesn't, since it moves the information into `Peer` instead of reinstating it into `CNode`.
* This was eventual since the underlying presumption that `CNode::IsAddrRelayPeer() == !CNode::IsBlockOnlyConn()` no longer holds true (also because `CNode::IsAddrRelayPeer()` doesn't exist anymore).
Special thanks to @UdjinM6 for help with understanding Dash-specifics with respect to functional tests through help on [dash#5964](https://github.com/dashpay/dash/pull/5964) and [dash#5967](https://github.com/dashpay/dash/pull/5967)
## Breaking Changes
None expected.
RPC changes have been introduced in `getnodeaddresses`, where a new input `network`, can filter addresses based on desired network and a new output, also `network`, will associate the address with the origin network. This change is expected to be backwards-compatible.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 1fedf470cd
Tree-SHA512: 533d33f79a0d9fd730073b3b9a58baf1dd3b0c95823e765c88a43cc974970ed3609bf1863c63ac7fc5586d1437e5250b0a2d3005468da09e407110a412bd0264
21ad71c578 Merge #21676: test: Use mocktime to avoid intermittent failure in rpc_tests (MarcoFalke)
76a41eb245 Merge #21602: rpc: add additional ban time fields to listbanned (MarcoFalke)
adea52a5fe Merge bitcoin-core/gui#260: Handle exceptions instead of crash (W. J. van der Laan)
7e023c394f Merge #17934: doc: Use CONFIG_SITE variable instead of --prefix option (fanquake)
bc6e3ed6e4 Merge #21606: fuzz: Extend psbt fuzz target a bit (MarcoFalke)
233fb245f7 Merge #21445: cirrus: Use SSD cluster for speedup (fanquake)
a224b800e4 Merge #21609: ci: increase CPU count of sanitizer job to increase memory limit (MarcoFalke)
ad947099a0 test: remove exception for util::Ref which doesn't exist more (Konstantin Akimov)
6674ee85ab Merge #21390: test: Test improvements for UTXO set hash tests (MarcoFalke)
e10eec249b Merge #21338: test: add functional test for anchors.dat (MarcoFalke)
d9c31d6817 Merge #21411: test: add logging, reduce blocks, move sync_all in wallet_ groups (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## Note for reviewers:
PRs bitcoin#17934 and bitcoin#21606 have been backported partially in past.
## What was done?
Removed unused sanitizer rules (see bitcoin#21366 and dashpay/dash#5055)
- bitcoin/bitcoin#21338
- bitcoin/bitcoin#21390
- bitcoin/bitcoin#21609
- bitcoin/bitcoin#21445
- bitcoin/bitcoin#21606
- bitcoin/bitcoin#17934
- bitcoin-core/gui#260
- bitcoin/bitcoin#21602
- bitcoin/bitcoin#21676
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 21ad71c578
Tree-SHA512: 94276d56255300d7d8c056d15b468720ba028d83cc585b16396e8bad90157e9e010490be239e09cccd4362f575f8df2d56dde3fb505745376c7790d70e3635cd
005a6b104a fix: format string in llmq/commitment - mismatched arguments (Konstantin Akimov)
4774e1e8f6 Merge #19809: log: Prefix log messages with function name and source code location if -logsourcelocations is set (Wladimir J. van der Laan)
43a94f0580 fix: adjust functional tests due to dash's support of thread name after v0.12 (Konstantin Akimov)
085120d9f9 Merge #20993: test: store subversion (user agent) as string in msg_version (MarcoFalke)
e866b43160 Merge #21542: ci: Bump macOS VM image to the latest version (fanquake)
a3702534e5 Merge #21354: build, doc: Drop no longer required packages from macOS cross-compiling dependencies (fanquake)
6bcc86ad3b Merge #21221: [tools] Allow argument/parameter bin packing in clang-format (MarcoFalke)
318c7263d0 Merge #19522: build: fix building libconsensus with reduced exports for Darwin targets (Wladimir J. van der Laan)
88a45d4a9a Merge #21138: ci: Re-run wine tests once if they fail (fanquake)
4abb768456 Merge #21126: ci: Properly bump to focal for win cross build (fanquake)
f254f77d75 Merge #21075: doc: Fix markdown formatting (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22 and related fixes
## What was done?
Follow-up fixes for bitcoin#19809
Backports:
- bitcoin/bitcoin#21075
- bitcoin/bitcoin#21126
- bitcoin/bitcoin#21138
- bitcoin/bitcoin#19522
- bitcoin/bitcoin#21221
- bitcoin/bitcoin#21354
- bitcoin/bitcoin#21542
- bitcoin/bitcoin#19809
- bitcoin/bitcoin#20993
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
Notice, that function name is included now by default to logs with `-logfunctionnames` and many logs have function name twice now such as:
```
node0 2024-04-06T20:13:56.564123Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.3] [masternode/sync.cpp:331] [NotifyHeaderTip] CMasternodeSync::NotifyHeaderTip -- pindexNew->nHeight: 5 fInitialDownload=0
```
For further development need to take it in account and do not use more direct calls `__func__` from code as well as reduce usages in codebase.
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 005a6b104a
Tree-SHA512: f98949c4605dda7d6dfe790554e1d31a8c8178b57520578fcd58178c68fe7af33c0d66a79865683c1357de9a73fa4f484eb828b28e11ca110b8e1915c1fcf9b3
a2f190deff Merge bitcoin-core/gui#115: Replace "Hide tray icon" option with positive "Show tray icon" one (Jonas Schnelli)
65b80e76b7 Merge #21531: test: remove qt byteswap compattests (MarcoFalke)
ba883c5da2 Merge bitcoin-core/gui#139: doc: Improve gui/src/qt README.md (MarcoFalke)
368c65d050 Merge bitcoin-core/gui#72: util: Log static plugins meta data and used style (Jonas Schnelli)
317777e7d7 Merge bitcoin-core/gui#171: Use layout manager for Create Wallet dialog (MarcoFalke)
83313a5603 Merge bitcoin#20789: Rework strong and weak net enum fuzzing (MarcoFalke)
4a3e3af6e7 Merge #20813: scripted-diff: Bump copyright headers (MarcoFalke)
e36eacd868 Merge #18772: rpc: calculate fees in getblock using BlockUndo data (MarcoFalke)
41a1e10954 Merge #20690: Clean up logging of outbound connection type (MarcoFalke)
648d6f04fb Merge bitcoin-core/gui#13: Hide peer detail view if multiple are selected (Jonas Schnelli)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## What was done?
- bitcoin-core/gui#13
- bitcoin-core/gui#115
- bitcoin/bitcoin#20690
- bitcoin/bitcoin#18772
- bitcoin/bitcoin#20813
- bitcoin/bitcoin#20789
- bitcoin-core/gui#171
- bitcoin-core/gui#72
- bitcoin-core/gui#139
- bitcoin/bitcoin#21531
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK a2f190deff
Tree-SHA512: 29421e7ca38583c47f49c2605775f34b64ae2fb6aeb45ac42941fbc78598fc26e7f7a248b40fcc2c9fd21154b0a6f2aed64287a8b7cca43de1b99ae3dccd990f
Since bitcoin#20267 changes default wallet in functional tests from legacy
wallets to descriptor wallets, we need to enforce --legacy-wallets for
functional tests that used protx which doesn't work yet for descriptor wallets
2e5f7def22e1b212fbd69e9147145d9d1f408aaf wallet, rpc: update listdescriptors response format (Ivan Metlushko)
Pull request description:
Update `listdescriptors` response format according to [RPC interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines).
This is a follow up for #20226
**Before:**
```
Result:
[ (json array) Response is an array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
```
**After:**
```
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"descriptors" : [ (json array) Array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
}
```
ACKs for top commit:
achow101:
re-ACK 2e5f7def22e1b212fbd69e9147145d9d1f408aaf
meshcollider:
utACK 2e5f7def22e1b212fbd69e9147145d9d1f408aaf
jonatack:
re-ACK 2e5f7def22e1b212fbd69e9147145d9d1f408aaf
Tree-SHA512: 49bf73e46e2a61003ce594a4bfc506eb9592ccb799c2909c43a1a527490a4b4009f78dc09f3d47b4e945d3d7bb3cd2632cf48c5ace5feed5066158cc010dddc1
de6b389d5db7b8426313c5be6fbd290f992c5aa8 tests: Test getaddressinfo parent_desc (Andrew Chow)
e4ac869a0a0083e2e3af3b56301bd5c8e0cf650b rpc: Add parent descriptor to getaddressinfo output (Andrew Chow)
bbe4a36152fb8d9c8c3682ca2380f1c88cca61cb wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow)
9be1437c49f986e8ed964d5f863b4bbcec340751 descriptors: Add ToNormalizedString and tests (Andrew Chow)
Pull request description:
Adds `parent_desc` field to the `getaddressinfo` RPC to export a public descriptor. Using the given address, `getaddressinfo` will look up which `DescriptorScriptPubKeyMan` can be used to produce that address. It will then return the descriptor for that `DescriptorScriptPubKeyMan` in the `parent_desc` field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets.
As part of this PR, a `ToNormalizedString` function is added to the descriptor classes. This really only has an effect on `BIP32PubkeyProvider`s that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.
ACKs for top commit:
Sjors:
utACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
S3RK:
Tested ACK de6b389
jonatack:
Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8 modulo a few minor comments
fjahr:
Code review ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
meshcollider:
Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
Backport notice: changes in feature_notification.py are missing due to #18878 is not done yet
49797c3ccfbb9f7ac9c1fbb574d35b315c103805 tests: Disable bdb dump test when no bdb (Andrew Chow)
1194cf9269e6c4bf67b09c4766f42bf173d12c0a Fix wallet_send.py wallet setup to work with descriptors (Andrew Chow)
fbaea7bfe44822710a36601c6b0febbd5e33dfbd Require legacy wallet for wallet_upgradewallet.py (Andrew Chow)
b1b679e0ab7a9981e3e78424fe8836edd59abf6f Explicitly mark legacy wallet tests as such (Andrew Chow)
09514e1bef46444a67cde9ff13e76bd4b9f8c7ac Setup wallets for interface_zmq.py (Andrew Chow)
4d03ef9a73ceb5ef4e9d184a135bca6bdeb8c311 Use MiniWallet in rpc_net.py (Andrew Chow)
4de23824b0c711ece68f9fc007ffac12126710aa Setup wallets for interface_bitcoin_cli.py (Andrew Chow)
7c71c627d28f0cddaf2349a55336278a681c27c2 Setup wallets with descriptors for feature_notifications (Andrew Chow)
1f1bef8dbab7225884d769a45477ee11d0ebf654 Have feature_filelock.py test both bdb and sqlite, depending on compiled (Andrew Chow)
c77975abc0123b29b0eb3481b8916e7c025b7c4c Disable upgrades tests that require BDB if BDB is not compiled (Andrew Chow)
1f20cac9d41e507901a2811d6db7147d7ab0321b Disable wallet_descriptor.py bdb format check if BDB is not compiled (Andrew Chow)
3641597d7ef6f5097a9e93cab3ef7e0f9c820296 tests: Don't make any wallets unless wallet is required (Andrew Chow)
b9b88f57a9b9a28e0f0614c12ae3012cf5050b10 Skip legacy wallet reliant tests if BDB is not compiled (Andrew Chow)
6f36242389bd3e7eacf594ce90491e8ccca70f3a tests: Set descriptors default based on compilation (Andrew Chow)
Pull request description:
This PR fixes tests for when BDB is not compiled. Tests which rely on or test legacy wallet behavior are disabled and skipped when BDB is not compiled. For the components of some tests that are for legacy wallet things, those parts of the tests are skipped.
For the majority of tests, changes are made so that they can be run with either legacy wallets or descriptor wallets without materially effecting the test. Most tests only need the wallet for balance and transactions, so the type of wallet is not an important part of those tests. Additionally, some tests are wallet agnostic and modified to instead use the test framework's MiniWallet.
ACKs for top commit:
laanwj:
ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805
ryanofsky:
Code review ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805. Only change since last review is dropping last commit. Previous review w/ suggestions for future followup is https://github.com/bitcoin/bitcoin/pull/20267#pullrequestreview-581508843
Tree-SHA512: 69659f8a81fb437ecbca962f4082c12835282dbf1fba7d9952f727a49e01981d749af9b09feda1c8ca737516c7d7a08ef17e782795df3fa69892d5021b41c1ed
4f2653a89018fa4d24bd2a551832a7410b682600 test: Use deterministic chain in utxo set hash test (Fabian Jahr)
4973c5175c5fd1f4791ea26e8ddefd6fb11ac1c3 test: Remove wallet dependency of utxo set hash test (Fabian Jahr)
1a27af1d7b5ec18b4248ead1eaf0f381047b4b24 rpc: Improve gettxoutsetinfo help (Fabian Jahr)
Pull request description:
Follow-ups to #19145:
- Small improvement on the help text of RPC gettxoutsetinfo
- Using deterministic blockchain in the test `functional/feature_utxo_set_hash.py`
- Removing wallet dependency in the test `functional/feature_utxo_set_hash.py`
Split out of #19521.
ACKs for top commit:
MarcoFalke:
review ACK 4f2653a89018fa4d24bd2a551832a7410b682600 👲
Tree-SHA512: 92927b3aa22b6324eb4fc9d346755313dec44d973aa69a0ebf80a8569b5f3a7cf3539721ebdba183737534b9e29b3e33f412515890f0d0b819878032a3bba8f9
581791c62067403fbeb4e1fd88c1d80549627c94 test: add functional test for anchors.dat (bruno)
Pull request description:
This PR adds a functional test for anchors.dat.
It creates a node and adds 2 outbound block-relay-only connections and 5 inbound connections.
When the node is down, anchors.dat should contain the 2 addresses from the outbound block-relay-only connections.
ACKs for top commit:
MarcoFalke:
Concept ACK 581791c62067403fbeb4e1fd88c1d80549627c94
hebasto:
ACK 581791c62067403fbeb4e1fd88c1d80549627c94
Tree-SHA512: 77038b09e36ee5ae473a26d6f566c0ed283af258c34df8486706a24f72b05abab621a293ac886d03849bc45bc28be7336137252225b25aff393baa6b5238688c
c62f9bc0e931f65eef63041d2c53f9a294c0e8d6 test: use fewer blocks in wallet_groups and move sync call (Jon Atack)
3a16b5ef95c1c25f8b78e591f985e80b41a6dbdd test: add missing logging to wallet_groups.py (Jon Atack)
Pull request description:
- add logging (particularly useful as the tests are somewhat slow)
- generate 101 blocks instead of 110
- move `sync_all` call into the loop, so fewer blocks are synced on each call, to hopefully see fewer CI timeouts as in https://bitcoinbuilds.org/index.php?ansilog=88eee99e-1727-44ed-b778-3b9c75c33928.log
```
L2742 File "/home/ubuntu/src/test/functional/wallet_groups.py", line 162, in run_test
L2743 self.sync_all()
test_framework.authproxy.JSONRPCException: 'syncwithvalidationinterfacequeue' RPC took longer than 960.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
ACKs for top commit:
MarcoFalke:
cr ACK c62f9bc0e931f65eef63041d2c53f9a294c0e8d6
Tree-SHA512: 711deafcd589cb8196cb207ff882e0f2ab6b70828a6abad91f83f535974cc430a56b9e8a960fd233d31d610932a0d48b49ee681aae564d145a3040288ecda8f8
b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)
Pull request description:
Prefix log messages with function name if `-logfunctionnames` is set.
Yes, exactly like `-logthreadnames` but for function names instead of thread names :)
This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.
For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)
Without any logging parameters:
```
$ src/bitcoind -regtest
2020-08-25T03:29:04Z Using RdRand as an additional entropy source
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z block tree size = 1
2020-08-25T03:29:04Z nBestHeight = 0
2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z 0 addresses found from DNS seeds
```
With `-logthreadnames` and `-logfunctionnames`:
```
$ src/bitcoind -regtest -logthreadnames -logfunctionnames
2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
```
ACKs for top commit:
laanwj:
Code review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08
MarcoFalke:
review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
de85af5cce727981383ac0fe81f635451b331f23 test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)
Pull request description:
It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.
Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing #19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.
So without this patch, we get:
```
$ jq . out.json | grep -m5 strSubVer
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
```
After this patch:
```
$ jq . out2.json | grep -m5 strSubVer
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
```
ACKs for top commit:
jnewbery:
utACK de85af5cce727981383ac0fe81f635451b331f23
Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
fa0074e2d82928016a43ca408717154a1c70a4db scripted-diff: Bump copyright headers (MarcoFalke)
Pull request description:
Needs to be done because no one has removed the years yet
ACKs for top commit:
practicalswift:
ACK fa0074e2d82928016a43ca408717154a1c70a4db
Tree-SHA512: 210e92acd7d400b556cf8259c3ec9967797420cfd19f0c2a4fa54cb2b3d32ad9ae27e771269201e7d554c0f4cd73a8b1c1a42c9f65d8685ca4d52e5134b071a3
fae32f295cc5b57c1cb95090bb60cddb42f9778a wallet: Add missing check for -descriptors wallet tool option (MarcoFalke)
faf8f61368696b9cbbea55ead30d6a48203235ff test: Add missing check for is_sqlite_compiled (MarcoFalke)
fa7dde1c418e2e700853bd30cc9e012c4e4c5ef2 wallet: Pass ArgsManager into ExecuteWalletToolFunc instead of using global (MarcoFalke)
Pull request description:
Also, fix a test failure when compiled without sqlite
ACKs for top commit:
ryanofsky:
Code review ACK fae32f295cc5b57c1cb95090bb60cddb42f9778a. Thanks for implementing the -descriptors check and dealing with the test failure!
jonatack:
Code review utACK fae32f295cc5b57c1cb95090bb60cddb42f9778a
Tree-SHA512: 3d7710694085822739a8316e4abc6db270799ca6ff6b0f9e5563ae240da65ae6a9cab7ba2647feae6ba540dac40b55b38ed41c8f6ed0bf02a3d1536284448927
23cac24dd3f2aaf88aab978e7ef4905772815cd2 tests: Test bitcoin-wallet dump and createfromdump (Andrew Chow)
a88c320041bd1cd1786b2dfd9ab698a67c2a57c6 wallettool: Add createfromdump command (Andrew Chow)
e1e7a90d5f0616a46ffadd62a9f1c65406cca6b4 wallettool: Add dump command (Andrew Chow)
Pull request description:
Adds two commands to the `bitcoin-wallet` tool: `dump` and `createfromdump`. These commands will be useful for a wallet storage migration in the future. It is also generally useful to have a storage agnostic dump like this. These commands are similar to BDB's `db_dump` and `db_load` tools. This can also be useful for manual construction of a wallet file for tests.
`dump` outputs every key-value pair from the wallet as comma separated hex. Each key-value pair is on its own line with the key and value in hex separated by a comma. This is output to the file specified by the new `-dumpfile` option.
`createfromdump` takes a file produced by `dump` and creates a new wallet file with exactly the records specified in that file.
A new option `-dumpfile` is added to the wallet tool. When used with `dump`, the records will be written to the specified file. When used with `createfromdump`, the file is read and the key-value pairs constructed from it. `createfromdump` requires `-dumpfile`.
A simple round-trip test is added to the `tool_wallet.py`.
This PR is based on #19334,
ACKs for top commit:
Sjors:
re-utACK 23cac24
MarcoFalke:
re review ACK 23cac24dd3f2aaf88aab978e7ef4905772815cd2 only change is rebase and removing useless shared_ptr wrapper 🎼
ryanofsky:
Code review ACK 23cac24dd3f2aaf88aab978e7ef4905772815cd2. Only changes since last review rebase and changing a pointer to a reference
Tree-SHA512: 2d63cf62baca3d16495aa698dc02f7d889c81b41015e9c92c23c275bb4a690fc176d351c3fd7f310bd6b17f5a936cc9be694cbecd702af741b96c0f530e72fa2
173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12 test: walettool create descriptors (Ivan Metlushko)
345e88eecf1b28607d5da3af38e19794a8a115ce wallettool: add param to create descriptors wallet (Ivan Metlushko)
6d3af3ab627096a824cb6a7ca1ebeddc7530361c wallettool: pass in DatabaseOptions into MakeWallet (Ivan Metlushko)
Pull request description:
Rationale: expose and promote descriptor wallets in more places; make cli tool more consistent with `createwallet` rpc.
Add `-descriptors` parameter which is off by default. When specified it will create a new descriptors wallet with sqlite backend, which is consistent with `createwallet` rpc.
This PR is based on a suggestion from **ryanofsky** https://github.com/bitcoin/bitcoin/pull/19137#discussion_r516779603
Example:
```
$ ./src/bitcoin-wallet -wallet=fewty -descriptors create
Topping up keypool...
Wallet info
===========
Name: fewty
Format: sqlite
Descriptors: yes
Encrypted: no
HD (hd seed available): yes
Keypool Size: 6000
Transactions: 0
Address Book: 0
```
```
$ ./src/bitcoin-wallet -wallet=fewty create
Topping up keypool...
Wallet info
===========
Name: fewty
Format: bdb
Descriptors: no
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2000
Transactions: 0
Address Book: 0
```
ACKs for top commit:
achow101:
ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12
ryanofsky:
Code review ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12. This seems pretty nicely implemented now, with opportunities to clean up more and dedup later
MarcoFalke:
Concept ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12 🌠
Tree-SHA512: cc32ba336ff709de2707ee15f495b4617908e8700ede8401a58e894f44cda485c544d644023c9a6604d88a62db9d92152383ee2e8abf691688c25cf6e222c622
66d012ad7f9381bacfc9b8388fa2ebf82cb86c9e test: RPC: getblock fee calculations (Elliott Jin)
bf7d6e31b1062ab5f90e14e83c56309f499fa2e9 RPC: getblock: tx fee calculation for verbosity 2 via Undo data (Elliott Jin)
Pull request description:
This change is progress towards #18771 . It adapts the fee calculation part of #16083 and addresses some feedback. The additional "verbosity level 3" features are planned for a future PR.
**Original PR description:**
> Using block undo data (like in #14802) we can now show fee information for each transaction in a block without the need for additional -txindex and/or a ton of costly lookups. For a start we'll add transaction fee information to getblock verbosity level 2. This comes at a negligible speed penalty (<1%).
ACKs for top commit:
luke-jr:
tACK 66d012ad7f9381bacfc9b8388fa2ebf82cb86c9e
fjahr:
tACK 66d012ad7f9381bacfc9b8388fa2ebf82cb86c9e
MarcoFalke:
review ACK 66d012ad7f9381bacfc9b8388fa2ebf82cb86c9e 🗜
Tree-SHA512: be1fe4b866946a8dc36427f7dc72a20e10860e320a28fa49bc85bd2a93a0d699768179be29fa52e18b2ed8505d3ec272e586753ef2239b4230e0aefd233acaa2
Backport notice:
- data have real blocks from testnet
- due to big gap in blocks before checkpoints (half-year) for sack of this test is added one more checkpoint, otherwise blocks are not accepted due to non-mockable time for other chains except regtest
- data for 2 blocks in split chain are generated locally for testnet
--------------
333317ce6b67aa92f7363d48cd750712190b4b6b test: Test that low difficulty chain fork is rejected (MarcoFalke)
fa31dc1bf4ee471c4641eef8de02702ba0619ae7 test: Pass down correct chain name in tests (MarcoFalke)
Pull request description:
To prevent OOM, Bitcoin Core will reject chain forks at low difficulty by default. This is the only use-case of checkpoints, so add a test for it to make sure the feature works as expected. If it didn't work, checkpoints would have no use-case and we might as well remove them
ACKs for top commit:
Sjors:
Thanks for adding the node 1 example. Code review ACK 333317c
Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
7e698732836121912f179b7c743a72dd6fdffa72 sync: remove DEBUG_LOCKCONTENTION preprocessor directives (Jon Atack)
9b08006bc502e67956d6ab518388fad6397cac8d log, sync: improve lock contention logging and add time duration (Jon Atack)
3f4c6b87f1098436693c4990f2082515ec0ece26 log, timer: add timing macro in usec LOG_TIME_MICROS_WITH_CATEGORY (Jon Atack)
b7a17444e0746c562ae97b26eba431577947b06a log, sync: add LOCK logging category, apply it to lock contention (Jon Atack)
Pull request description:
To enable lock contention logging, `DEBUG_LOCKCONTENTION` has to be defined at compilation. Once built, the logging is not limited to a category and is high frequency, verbose and in all-caps. With these factors combined, it seems likely to be rarely used.
This patch:
- adds a `lock` logging category
- adds a timing macro in microseconds, `LOG_TIME_MICROS_WITH_CATEGORY`
- updates `BCLog::LogMsg()` to omit irrelevant decimals for microseconds and skip unneeded code and math
- improves the lock contention logging, drops the all-caps, and displays the duration in microseconds
- removes the conditional compilation directives
- allows lock contentions to be logged on startup with `-debug=lock` or at run time with `bitcoin-cli logging '["lock"]'`
```
$ bitcoind -signet -debug=lock
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 completed (4μs)
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 completed (4μs)
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 started
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 completed (20μs)
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 started
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 completed (3μs)
$ bitcoin-cli -signet logging
"lock": true,
$ bitcoin-cli -signet logging [] '["lock"]'
"lock": false,
$ bitcoin-cli -signet logging '["lock"]'
"lock": true,
```
I've tested this with Clang 13 and GCC 10.2.1, on Debian, with and without `--enable-debug`.
ACKs for top commit:
hebasto:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72, added a contention duration to the log message since my [previous](https://github.com/bitcoin/bitcoin/pull/22736#pullrequestreview-743764606) review.
theStack:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72 🔏⏲️
Tree-SHA512: c4b5eb88d3a2c051acaa842b3055ce30efde1f114f61da6e55fcaa27476c1c33a60bc419f7f5ccda532e1bdbe70815222ec2b2b6d9226f29c8e94e598aacfee7
The value of nServices for `NODE_NETWORK | NODE_WITNESS`, the default for
p2p_addr{v2}_relay.py in Bitcoin Core, is 9. Dash doesn't implement SegWit
and so the corresponding nServices value is `NODE_NETWORK`, which is 1.
95487b055328b590ba83f258de9637ab0f9a2f17 doc: Drop mentions of Travis CI as it is no longer used (Hennadii Stepanov)
09d105ef0f8b4b06bf248721a1209c9e16e9db75 ci: Drop travis_fold feature as Travis CI is no longer used (Hennadii Stepanov)
Pull request description:
As Travis CI is no longer used, this PR:
- drops `travis_fold` feature
- drops mentions of Travis CI in docs
ACKs for top commit:
MarcoFalke:
ACK 95487b055328b590ba83f258de9637ab0f9a2f17
Tree-SHA512: 2e259bb8b1e37bcefc1251737bb2716f06ddb57c490010b373825c4e70f42ca38efae69a2f63f21f577d7cee3725b94097bdddbd313f8ebf499281cf97c53cef
d5d1a714fb Merge bitcoin/bitcoin#24390: test: Remove suppression no longer needed with headers-only Boost.Test (fanquake)
51630d2e5e Merge bitcoin/bitcoin#22824: refactor: remove RecursiveMutex cs_nBlockSequenceId (MarcoFalke)
a9b1575fe8 Merge bitcoin/bitcoin#22781: wallet: fix the behavior of IsHDEnabled, return false in case of a blank hd wallet. (Samuel Dobson)
0505229c89 Merge bitcoin/bitcoin#22327: cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
1dc97c7679 Merge bitcoin/bitcoin#22149: test: Add temporary logging to debug #20975 (W. J. van der Laan)
44f91cbc9a Merge #21597: test: Document race:validation_chainstatemanager_tests suppression (fanquake)
c326830f48 Merge bitcoin-core/gui#243: fix issue when disabling the auto-enabled blank wallet checkbox (MarcoFalke)
267f42fd6a Merge #21382: build: Clean remnants of QTBUG-34748 fix (fanquake)
1fcc5f1101 Merge #20540: test: Fix wallet_multiwallet issue on windows (MarcoFalke)
4afbaf2ea1 Merge #20322: test: Fix intermittent issue in wallet_listsinceblock (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Batch of backports
## What was done?
Trivial batch of backports
## How Has This Been Tested?
CI looks good
## Breaking Changes
None
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: 8eeac54f011eb1111888c745dd56184ac9601de290f2b0f7b7ad02240e8dc1cab5a47fed26bfed2bd6f1066e0710827a3e5b2426f0bf66821cf1cd09099d5160
faa94961d6e38392ba068381726ed4e033367b03 test: Add temporary logging to debug #20975 (MarcoFalke)
Pull request description:
to be reverted after a fix
ACKs for top commit:
laanwj:
Code review ACK faa94961d6
Tree-SHA512: 1f3103fcf4cad0af54e26c4d257bd824b128b5f2d2b81c302e861a829fd55d6a099fa476b79b30a71fe98975ae604b9e3ff31fd48a51d442389a9bd515e60ba0
fada2dfcac1c4b47ee76b877d91d515cf1d36410 test: Fix wallet_multiwallet issue on windows (MarcoFalke)
Pull request description:
The error message on windows:
> 2020-11-30T18:10:47.536032Z ListWalletDir: Error scanning C:\Users\user\AppData\Local\Temp\test_runner_₿_🏃_20201130_181042\wallet_multiwallet_0\node0\regtest\wallets\self_walletdat_symlink: boost::filesystem::status: The name of the file cannot be resolved by the system: "C:\Users\user\AppData\Local\Temp\test_runner_₿_🏃_20201130_181042\wallet_multiwallet_0\node0\regtest\wallets\self_walletdat_symlink\wallet.dat"
ACKs for top commit:
promag:
Code review ACK fada2dfcac1c4b47ee76b877d91d515cf1d36410. Although it could ignore (don't log) directories that lead to no permission error.
fanquake:
ACK fada2dfcac1c4b47ee76b877d91d515cf1d36410
Tree-SHA512: b475162cc3cd1574209d916605b229a79c8089714295f5e16569b71f958f0007d54dc76833938492d931387784588b11b73e3ef00f963540af42c079417f8d72
fa108d6a757838225179a8df942cfb6d99c98c90 test: update tests for peer discouragement (Jon Atack)
1a9f462caa63fa16d7b4415312d2032a42b3fe0b gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack)
Pull request description:
This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning.
- no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per https://github.com/bitcoin/bitcoin/pull/19464#pullrequestreview-447452052
- update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per https://github.com/bitcoin/bitcoin/pull/19464#issuecomment-658052518
ACKs for top commit:
jnewbery:
ACK fa108d6a757838225179a8df942cfb6d99c98c90
laanwj:
ACK fa108d6a757838225179a8df942cfb6d99c98c90
Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50
41d55d30579358c805036201664ad6a1c1d48681 doc: getpeerinfo banscore deprecation release note (Jon Atack)
dd54e3796e633cfdf6954af306afd26eadc25116 test: getpeerinfo banscore deprecation test (Jon Atack)
8c7647b3fbbab03ea84071cf3cd2d0d2bf8be255 rpc: deprecate banscore field in rpc getpeerinfo (Jon Atack)
Pull request description:
Per https://github.com/bitcoin/bitcoin/pull/19219#discussion_r443074487 and https://github.com/bitcoin/bitcoin/pull/19219#issuecomment-652699592, this PR deprecates returning the `banscore` field in the `getpeerinfo` RPC, updates the help, adds a test, and updates the release notes. Related to #19464.
ACKs for top commit:
fanquake:
ACK 41d55d30579358c805036201664ad6a1c1d48681
Tree-SHA512: 8eca08332581e2fe191a2aafff6ba89ce39413f0491ed0de8b86577739f0ec430b1a8fbff2914b0f3138a229563dfcc1981c0cf5b7dd6061b5c48680a28423bc
bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner)
191405420815d49ab50184513717a303fc2744d6 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner)
a79396fe5f8f81c78cf84117a87074c6ff6c9d95 test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner)
2ce7b47958c4a10ba20dc86c011d71cda4b070a5 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner)
Pull request description:
There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`.
Instances were found via
* `git grep "deserialize.*BytesIO"`
and some of them manually, when it were not one-liners.
Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion https://github.com/bitcoin/bitcoin/pull/22257#discussion_r652404782)
ACKs for top commit:
MarcoFalke:
review re-ACK bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb 😁
Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
b9e76f1bf08c52fcd402b2314e00db4ad247ebc8 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7dc9a7b0c55d28011161606399417664 rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef698a1695050f82ef6567b3b87a21861 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59fe51473571661316214156fbdbdcae rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8
promag:
ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
1f449586a9e39bc4fb53cb5c7a31362e47aea19b test: add `bad-txns-prevout-null` test to mempool_accept.py (Sebastian Falbesoner)
aa0a5bb70d77739d43d5a9ceae78fb0c6fafd435 test: add `bad-txns-prevout-null` test case to invalid_txs.py (Sebastian Falbesoner)
Pull request description:
This simple PR adds missing tests for the reject reason `bad-txns-prevout-null`, which is thrown in the function `CheckTransaction()`: a62fc35a15/src/consensus/tx_check.cpp (L52-L54)
Basically this condition is met for non-coinbase transactions (the code snippet above only hits if `!tx.IsCoinBase()`) with coinbase-like outpoints, i.e. hash=0, n=0xffffffff.
Can be tested by running the functional tests `feature_block.py`, `p2p_invalid_tx.py` and `mempool_accept.py`. Not sure if the redundancy in the tests is desired (I guess it would make sense if the mempool acceptance test also makes use of the invalid_txs templates?).
ACKs for top commit:
rajarshimaitra:
tACK 1f449586a9
brunoerg:
tACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b
kristapsk:
ACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b, code looks correct and all tests pass.
Tree-SHA512: 2d4f940a6ac8e0d80d2670c9e1111cbf43ae6ac62809a2ccf17cffee9a41d387ea4d889ee300eb4a407c055b13bfa5d37102a32ed59964a9b6950bd907ba7204
5a6b8b6b1f partial Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX with avoidpartialspends (fanquake)
2f788aa76d fix: change port to use for zmq in interface_zmq_dash.py (Konstantin Akimov)
0ce66fd477 Merge #19507: Expand functional zmq transaction tests (Wladimir J. van der Laan)
a1c2386153 Merge #17445: zmq: Fix due to invalid argument and multiple notifiers (Wladimir J. van der Laan)
44929bad82 Merge #16404: qa: Test ZMQ notification after chain reorg (MarcoFalke)
1707f01309 fix: follow-up changes from bitcoin/bitcoin#22220 for maxapsfee (Konstantin Akimov)
eb4270deae Merge #19743: -maxapsfee follow-up (Samuel Dobson)
6a6d379711 Merge #19756: tests: add sync_all to fix race condition in wallet groups test (MarcoFalke)
5821a1d23a Merge #14582: wallet: always do avoid partial spends if fees are within a specified range (Samuel Dobson)
59d5a4ef39 Merge #19773: wallet: Avoid recursive lock in IsTrusted (Samuel Dobson)
2489f29f0e Merge #19830: test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles (fanquake)
10fa7a66b6 Merge #19538: ci: Add tsan suppression for race in DatabaseBatch (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v21
## What was done?
- bitcoin/bitcoin#19538
- bitcoin/bitcoin#19830
- bitcoin/bitcoin#19773
- bitcoin/bitcoin#14582
- bitcoin/bitcoin#19756
- bitcoin/bitcoin#19743
- bitcoin/bitcoin#16404
- bitcoin/bitcoin#17445
- bitcoin/bitcoin#19507
- partial bitcoin/bitcoin#27053
+some extra fixes and missing changes from bitcoin/bitcoin#22220 for `maxapsfee`
+changed port for zmq in `interface_zmq_dash.py` to prevent intermittent error in `interface_zmq.py`
## How Has This Been Tested?
Run unit & functional tests
## Breaking Changes
`CreateTransaction` now uses sometime 2 private keys for one transaction instead one
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 5a6b8b6b1f
Tree-SHA512: 7efd8a31808f155c08035d0fb7ceaac369e3e44e68d2c91a88e52815a60efba5fe9458f41d93352627c2c062d414fb0207dcf216fa75b54af210b503f9123de6
14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27051
When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain.
If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected.
I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582
ACKs for top commit:
achow101:
ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e
Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
7356292e1d7a44da8a2bd31c02c58d550bf38009 Have zmq reorg test cover mempool txns (Gregory Sanders)
a0f4f9c983e57cc97ecbc56d0177eaf1854c842c Add zmq test for transaction pub during reorg (Gregory Sanders)
2399a0600ca9c4b676fa2f97520b8ecc44642246 Add test case for mempool->block zmq notification (Gregory Sanders)
e70512a83c69bc85e96b08ade725594eda3e230f Make ordering of zmq consumption irrelevant to functional test (Gregory Sanders)
Pull request description:
Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter.
Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons.
Remaining confusion:
1) Notification patterns seem to vary wildly with the inclusion of mempool transactions being reorg'ed. See difference between "Add zmq test for transaction pub during reorg" and "Have zmq reorg test cover mempool txns" commits for specifics.
2) Why does a reorg'ed transaction get announced 3 times? From what I understand it can get announced once for disconnected block, once for mempool entry. What's the third? It occurs a 4th time when included in a block(not added in test)
ACKs for top commit:
laanwj:
code review ACK 7356292e1d7a44da8a2bd31c02c58d550bf38009
promag:
Code review ACK 7356292e1d7a44da8a2bd31c02c58d550bf38009.
Tree-SHA512: 573662429523fd6a1af23dd907117320bc68cb51a93fba9483c9a2160bdce51fb590fcd97bcd2b2751d543d5c1148efa4e22e1c3901144f882b990ed2b450038
3e730bf90aaf53c41ff3a778f6aac15d163d1c0c zmq: Fix due to invalid argument and multiple notifiers (João Barbosa)
Pull request description:
ZMQ initialization is interrupted if any notifier fails, and in that case all notifiers are destroyed. The notifier shutdown assumes that the initialization had occurred. This is not valid when there are multiple notifiers and any except the last fails to initialize.
Can be tested by running test/functional/interface_zmq.py from this branch with bitcoind from master.
Closes#17185.
ACKs for top commit:
laanwj:
Code review ACK 3e730bf90aaf53c41ff3a778f6aac15d163d1c0c, thanks for adding a test
Tree-SHA512: 5da710e97dcbaa94896d019e75162d470f6d381ee07c60e5b3e9db93d11e8f7ca9bf2c509efa4486199e88c96c3e720cc96b4e35b62725d4c7db8e8e9bf6e09d
72ae20fc142457a200278cb2fedc5e32a3766b58 tests: add sync_all to fix race condition in wallet groups test (Karl-Johan Alm)
Pull request description:
This most likely fixes#19749, the intermittent CI issues with wallet_groups.
This fix is also included in #19743.
Top commit has no ACKs.
Tree-SHA512: dd6ef7f89829483e2278191c21fe0912b51fd2187c10a0fa158339c5ab9f22d93b733ae10f17ef25d8b64f44e596e66dba8d7db5c009343472f422ce4cd67d8f
7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf696c53f22536f9ca2e51949c164f6b06 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
achow101:
ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
fa3d9ce3254882c545d700990fe8e9a678f31eed rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke)
fa32c1d5ec25bc53bf989a8ae68e688593d2859d rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke)
faaa46dc204d6d714f71dbc6f0bf02215dba0f0f rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke)
fa93bc14c7411a108dd024d391344fabf0f76369 rpc: Remove unused return type from appendCommand (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:
fjahr:
tested ACK fa3d9ce3254882c545d700990fe8e9a678f31eed
promag:
Code review ACK fa3d9ce3254882c545d700990fe8e9a678f31eed.
Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
f0aa8aeea5a183ea44a877255d12db7732f2e0a8 test: add rpc_generate functional test (Jon Atack)
92d94ffb8d07cc0d2665c901de5903a3a90d5fd0 rpc: print useful help and error message for generate (Jon Atack)
8d32d2011d3f4e1d9e587d6f80dfa4a3e9f9393d test: consider generate covered in _get_uncovered_rpc_commands() (Jon Atack)
Pull request description:
This was a requested follow-up to #19133 and #17700 to alleviate confusion and head-scratching by people following tutorials that use `generate`. See https://github.com/bitcoin/bitcoin/pull/19455#issuecomment-668172916 below, https://github.com/bitcoin/bitcoin/pull/19133#issuecomment-636860943 and https://github.com/bitcoin/bitcoin/pull/17700#issuecomment-566159096.
before
```
$ bitcoin-cli help generate
help: unknown command: generate
$ bitcoin-cli generate
error code: -32601
error message:
Method not found
```
after
```
$ bitcoin-cli help generate
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.
$ bitcoin-cli generate
error code: -32601
error message:
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.
```
In the general help it remains hidden, as requested by laanwj.
```
$ bitcoin-cli help
== Generating ==
generateblock "output" ["rawtx/txid",...]
generatetoaddress nblocks "address" ( maxtries )
generatetodescriptor num_blocks "descriptor" ( maxtries )
```
ACKs for top commit:
adamjonas:
utACK f0aa8aeea5a183ea44a877255d12db7732f2e0a8
pinheadmz:
ACK f0aa8aeea5a183ea44a877255d12db7732f2e0a8
Tree-SHA512: d083652589ad3e8228c733455245001db22397559c3946e7e573cf9bd01c46e9e88b72d934728ec7f4361436ae4c74adb8f579670b09f479011924357e729af5
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 fa77de2baa
ryanofsky:
Code review ACK fa77de2baa40ee828c850ef4068c76cc3619e87b. Pretty straightfoward changes
Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3
9c54cb16de Merge #19405: rpc, cli: add network in/out connections to `getnetworkinfo` and `-getinfo` (Wladimir J. van der Laan)
19aba38cab Merge #19200: rpc: remove deprecated getaddressinfo fields (Samuel Dobson)
f5642281cc Merge #20282: wallet: change upgradewallet return type to be an object (Samuel Dobson)
Pull request description:
## Issue being fixed or feature implemented
Bitcoin backports with breaking changes
## What was done?
- bitcoin/bitcoin#20282
- bitcoin/bitcoin#19200
- bitcoin/bitcoin#19405
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
### RPC:
- `upgradewallet` now returns object for future extensibility (#20282)
- `getnetworkinfo` now returns fields `connections_in`, `connections_out`,
`connections_mn_in`, `connections_mn_out`, `connections_mn`
that provide the number of inbound and outbound peer
connections. These new fields are in addition to the existing `connections`
field, which returns the total number of peer connections. Old fields
`inboundconnections`, `outboundconnections`, `inboundmnconnections`,
`outboundmnconnections` and `mnconnections` are removed (#19405)
- Backwards compatibility has been dropped for two `getaddressinfo` RPC
deprecations, as notified in the 19.1.0 and 19.2.0 release notes.
The deprecated `label` field has been removed as well as the deprecated `labels` behavior of
returning a JSON object containing `name` and `purpose` key-value pairs. Since
20.1, the `labels` field returns a JSON array of label names. (#19200)
### CLI
- The `connections` field of `bitcoin-cli -getinfo` is expanded to return a JSON
object with `in`, `out` and `total` numbers of peer connections and `mn_in`,
`mn_out` and `mn_total` numbers of verified mn connections. It previously
returned a single integer value for the total number of peer connections. (#19405)
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 9c54cb16de
Tree-SHA512: 9710cdd062d02d64e2eebcecca1b5c2e6ccda5ca6d9bd6d1833700f4273fcfb206ae99134f71bbc8b0843cb8ebba208c72139f5a624d79ec7362bd73b117bfb2
581b343d5bf517510ab0236583ca96628751177d Add in/out connections to cli -getinfo (Jon Atack)
d9cc13e88d096c1a171159c01cbb96444f7f8d7f UNIX_EPOCH_TIME fixup in rpc getnettotals (Jon Atack)
1ab49b81cf32b6ef9e312a0a8ac45c68a3262f0d Add in/out connections to rpc getnetworkinfo (Jon Atack)
Pull request description:
This is basic info that is present in the GUI that I've been wishing to have exposed via the RPC and CLI without needing a bash workaround or script. For human users it would also be useful to have it in `-getinfo`.
`bitcoin-cli getnetworkinfo`
```
"connections": 15,
"connections_in": 6,
"connections_out": 9,
```
`bitcoin-cli -getinfo`
```
"connections": {
"in": 6,
"out": 9,
"total": 15
},
```
Update the tests, RPC help, and release notes for the changes. Also fixup the `getnettotals` timemillis help while touching `rpc/net.cpp`.
-----
Reviewers can manually test this PR by [building from source](https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests), launching bitcoind, and then running `bitcoin-cli -getinfo`, `bitcoin-cli getnetworkinfo`, `bitcoin-cli help getnetworkinfo`, and `bitcoin-cli help getnettotals` (for the UNIX epoch time change).
ACKs for top commit:
eriknylund:
> tACK [581b343](581b343d5b) on master at [a0a422c](a0a422c34c), ran unit & functional tests and and confirmed changes on an existing datadir ✌️
benthecarman:
tACK `581b343`
willcl-ark:
tACK for 581b343d5bf517510ab0236583ca96628751177d, this time rebased onto master at 862fde88be706adb20a211178253636442c3ae00.
shesek:
tACK `581b343`. This provides what I needed, thanks!
n-thumann:
tACK 581b343 on master at a0a422c, ran unit & functional tests and and confirmed changes on an existing datadir ✌️
Tree-SHA512: 08dd3ac8fefae401bd8253ff3ac027603c528eeccba53cedcb127771316173a7052fce44af8fa33ac98ebc4cf2a2b11cdefd949995d55e9b9a5942b876d00dc5
BACKPORT NOTICE:
These backports #17578 and #17585 are included to 19.1 and 19.2. That's long enough!
------------------------------------------
bc01f7ae0538d3c647ce8dfbc29f7914d5df3fbb doc: release note for rpc getaddressinfo removals (Jon Atack)
90e989390ee50633fff0e4f210a1ea23ff00e012 rpc: getaddressinfo RPCResult fixup (Jon Atack)
a8507c99da10791aa69ca277128e06753942e976 rpc: remove deprecated getaddressinfo `labels: purpose` (Jon Atack)
645a8653c895e4fc7717e9e5ac045612b5deaa60 rpc: remove deprecated getaddressinfo `label` field (Jon Atack)
Pull request description:
These were deprecated in #17578 and #17585, with expected 0.21 removal notified in the 0.20 release notes.
```
- The `getaddressinfo` RPC has had its `label` field deprecated
(re-enable for this release using the configuration parameter
`-deprecatedrpc=label`). The `labels` field is altered from returning
JSON objects to returning a JSON array of label names (re-enable
previous behavior for this release using the configuration parameter
`-deprecatedrpc=labelspurpose`). Backwards compatibility using the
deprecated configuration parameters is expected to be dropped in the
0.21 release. (#17585, #17578)
```
ACKs for top commit:
Sjors:
utACK bc01f7a
adamjonas:
utACK bc01f7a
meshcollider:
utACK bc01f7ae0538d3c647ce8dfbc29f7914d5df3fbb
Tree-SHA512: ae1af381e32c4c3bde8b061a56382838513a9a82c88767843cdeae3a2ab8aa7d8c2e66e106d2b31ea07d74bb80c191a2f842c9aaecc7c5438ad9a9bc66d1b251
0c845e3f8995eb8dc543a63899e5633a46091b4e test: Fix wallet_listdescriptors.py if bdb is not compiled (Hennadii Stepanov)
Pull request description:
If build system is configured `--without-bdb`, the `wallet_listdescriptors.py` fails:
```
$ test/functional/wallet_listdescriptors.py --descriptors
2021-07-14T13:20:52.931000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_02p7o1c9
2021-07-14T13:21:23.377000Z TestFramework (INFO): Test that the command is not available for legacy wallets.
2021-07-14T13:21:23.381000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
self.run_test()
File "test/functional/wallet_listdescriptors.py", line 34, in run_test
node.createwallet(wallet_name='w1', descriptors=False)
File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_node.py", line 685, in createwallet
return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)
File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Compiled without bdb support (required for legacy wallets) (-4)
2021-07-14T13:21:23.436000Z TestFramework (INFO): Stopping nodes
2021-07-14T13:21:24.092000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_02p7o1c9
2021-07-14T13:21:24.092000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_02p7o1c9/test_framework.log
2021-07-14T13:21:24.092000Z TestFramework (ERROR):
2021-07-14T13:21:24.092000Z TestFramework (ERROR): Hint: Call /home/hebasto/GitHub/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_02p7o1c9' to consolidate all logs
2021-07-14T13:21:24.092000Z TestFramework (ERROR):
2021-07-14T13:21:24.092000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2021-07-14T13:21:24.092000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2021-07-14T13:21:24.092000Z TestFramework (ERROR):
```
This PR fixes this issue.
Also see #20267.
ACKs for top commit:
achow101:
ACK 0c845e3f8995eb8dc543a63899e5633a46091b4e
Tree-SHA512: d7116a9ae30c7b7e3f55f55d2eea66f9e293c38d6757ed66d0477e4256ff5fedca5ddedafa0ef0c09f4dc1f7f973163e5a46090da26b067fdddbd9ea2ee76633
c7b7e0a69265946aecc885be911c7650911ba2e3 tests: Make only desc wallets for wallet_multwallet.py --descriptors (Andrew Chow)
d4b67ad214ada7645c4ce2d5ec336fe5c3f7f7ca Avoid creating legacy wallets in wallet_importdescriptors.py (Andrew Chow)
6c9c12bf87f95066acc28ea2270a00196eb77703 Update feature_backwards_compatibility for descriptor wallets (Andrew Chow)
9a4c631e1c00eb1661c000978b133d7aa0226290 Update wallet_labels.py to not require descriptors=False (Andrew Chow)
242aed7cc1d003e8fed574bbebd19c7e54e23402 tests: Add a --legacy-wallet that is mutually exclusive with --descriptors (Andrew Chow)
388053e1722632c2e485c56a444bc75cf0152188 Disable some tests for tool_wallet when descriptors (Andrew Chow)
47d3243160fdec7e464cfb8f869be7f5d4ee25fe Make raw multisig tests legacy wallet only in rpc_rawtransaction.py (Andrew Chow)
59d3da5bce4ebd9c2291d8f201a53ee087938b21 Do addmultisigaddress tests in legacy wallet mode in wallet_address_types.py (Andrew Chow)
25bc5dccbfd52691adca6edd418dd54290300c28 Use importdescriptors when in descriptor wallet mode in wallet_createwallet.py (Andrew Chow)
0bd1860300b13b12a25d330ba3a93ff2d13aa379 Avoid dumpprivkey and watchonly behavior in rpc_signrawtransaction.py (Andrew Chow)
08067aebfd7e838e6ce6b030c31a69422260fc6f Add script equivalent of functions in address.py (Andrew Chow)
86968882a8a26312a7af29c572313c4aff488c11 Add descriptor wallet output to tool_wallet.py (Andrew Chow)
3457679870e8eff2a7d14fe59a479692738c48b6 Use separate watchonly wallet for multisig in feature_nulldummy.py (Andrew Chow)
a42652ec10c733a5bf37e418e45d4841f54331b4 Move import and watchonly tests to be legacy wallet only in wallet_balance.py (Andrew Chow)
4b871909d6e4a51888e062d322bf53263deda15e Use importdescriptors for descriptor wallets in wallet_bumpfee.py (Andrew Chow)
c2711e4230d9a423ead24f6609691fb338b1d26b Avoid dumpprivkey in wallet_listsinceblock.py (Andrew Chow)
553dbf9af4dea96e6a3e79bba9607003342029bd Make import tests in wallet_listtransactions.py legacy wallet only (Andrew Chow)
dc81418fd01021070f3f66bab5fee1484456691a Use a separate watchonly wallet in rpc_fundrawtransaction.py (Andrew Chow)
a357111047411f18c156cd34a002a38430f2901c Update wallet_importprunedfunds to avoid dumpprivkey (Andrew Chow)
Pull request description:
I went through all the tests and checked whether they passed with descriptor wallets. This partially informed some changes in #16528. Some tests needed changes to work with descriptor wallets. These were primarily due to import and watchonly behavior. There are some tests and test cases that only test legacy wallet behavior so those tests won't be run with descriptor wallets.
This PR updates more tests to have to the `--descriptors` switch in `test_runner.py`. Additionally a mutually exclusive `--legacy-wallet` option has been added to force legacy wallets. This does nothing currently but will be useful in the future when descriptor wallets are the default. For the tests that rely on legacy wallet behavior, this option is being set so that we don't forget in the future. Those tests are `feature_segwit.py`, `wallet_watchonly.py`, `wallet_implicitsegwit.py`, `wallet_import_with_label.py`, and `wallet_import_with_label.py`.
If you invert the `--descriptors`/`--legacy-wallet` default so that descriptor wallets are the default, all tests (besides the legacy wallet specific ones) will pass.
ACKs for top commit:
MarcoFalke:
review ACK c7b7e0a69265946aecc885be911c7650911ba2e3 🎿
laanwj:
ACK c7b7e0a69265946aecc885be911c7650911ba2e3
Tree-SHA512: 2f4e87815005d1d0a2543ea7947f7cd7593d8cf5312228ef85f8e096f19739b225769961943049cb44f6f07a35b8de988e2246ab9aca5bb5a0b2e62694d5637d
647b81b70938dc4dbcf32399c56f78be395c721a wallet, rpc: add listdescriptors command (Ivan Metlushko)
Pull request description:
Looking for concept ACKs
**Rationale**: allow users to inspect the contents of their newly created descriptor wallets.
Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it:
* add an option to export xprv
* with #19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes
The output is compatible with `importdescriptors` command so it could be easily used for backup/recover purposes.
**Output example:**
```json
[
{
"desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h",
"timestamp": 1296688602,
"active": false,
"range": [
0,
999
],
"next": 0
}
]
```
ACKs for top commit:
jonatack:
re-ACK 647b81b70938dc4dbcf32399c56f78be395c721a rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet)
achow101:
re-ACK 647b81b
Tree-SHA512: 51a3620bb17c836c52cecb066d4fa9d5ff418af56809046eaee0528c4dc240a4e90fff5711ba96e399c6664e00b9ee8194e33852b1b9e75af18061296e19a8a7
a66a7a1a7060bb422eba3b8c214852416c4280d1 walletdb: don't reinitialize desc cache with multiple cache entries (Andrew Chow)
Pull request description:
When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works.
ACKs for top commit:
hugohn:
tACK [a66a7a1](a66a7a1a70)
jonatack:
ACK a66a7a1a706
meshcollider:
Code review ACK a66a7a1a7060bb422eba3b8c214852416c4280d1
Tree-SHA512: 3df746421a008708eaa3bbbdd12b9ddd3e2ec111d54625a212dca7414b971cc1f6e2b1757b3232c31a2f637d1b1ef43bf3ffa4ac4216646cf1e92db5f79954f1
3a83a01694160f2e722e1bc90a328bd569b8e109 [tests] move generate_wif_key to wallet_util.py (John Newbery)
b216b0b71f7853d747af8b712fc250c699f3c320 [tests] sort imports in rpc_createmultisig.py (John Newbery)
e38081846d889fcbbbc6ca4a4d3bca26807fde2f Revert "[TESTS] Move base58 to own module to break circular dependency" (John Newbery)
Pull request description:
generate_wif_key is a wallet utility function. Move it from the EC key module to the wallet util module.
This fixes the circular dependency issue in #17977
ACKs for top commit:
MarcoFalke:
ACK 3a83a01694160f2e722e1bc90a328bd569b8e109 🍪
Tree-SHA512: 24985dffb75202721ccc0c6c5b52f1fa5d1ce7963bccde24389feb913cab4dad0c265274ca67892c46c8b64e6a065a0f23263a89be4fb9134dfefbdbe5c7238a
c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0 [TESTS] Move base58 to own module to break circular dependency (Pieter Wuille)
Pull request description:
I encountered difficulties with the test framework in #17977. This fixes them, and I think the change is generally useful.
ACKs for top commit:
laanwj:
Code review ACK c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0
MarcoFalke:
ACK c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0 according to --color-moved=dimmed-zebra this is a move-only apart from the imports 👒
Tree-SHA512: 9e0493de3e279074f0c70e92c959b73ae30479ad6f2083a3c6bbf4b0191d65ef94854559a5b7c904f5dadc5e93129ed00f6dc0a8ccce6ba7921cd45f7119f74b
facede18a42ccbf45cb5156bd4e5c9cabb359821 test: Check that invalid witness destinations can not be imported (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK facede18a42ccbf45cb5156bd4e5c9cabb359821 -- thanks for adding!
Tree-SHA512: 87000606fac2e6f2780ca75cdeeb2dc1f0528d9b8f13e4156e8304ce7a6b1eb014781b6f0c59d11544bf360ba3dc5f99549470b0876132e189b9107f2c6bb38d
2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64 [wallet] Return object from upgradewallet RPC (Sishir Giri)
Pull request description:
Change the return type of upgradewallet to be an object for future extensibility.
Also return any error string returned from the `UpgradeWallet()` function.
ACKs for top commit:
MarcoFalke:
ACK 2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64
meshcollider:
Tested ACK 2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64
Tree-SHA512: bcc7432d2f35093ec2463ea19e894fa885b698c0e8d8e4bd2f979bd4d722cbfed53ec589d6280968917893c64649dc9e40800b8d854273b0f9a1380f51afbdb1
e073f1dfda7a2a2cb2be9fe2a1d576f122596021 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6)
367bb7a80cc71130995672c853d4a6e0134721d6 wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6)
Pull request description:
I think the expected behaviour of `getrawchangeaddress` and `getnewaddress` RPCs is that their failure should not affect keypool in any way. At least that's how legacy wallets work, you can confirm this behaviour by running `wallet_keypool.py --legacy-wallet` on master with e073f1dfda7a2a2cb2be9fe2a1d576f122596021 applied on top. However running `wallet_keypool.py --descriptors` on the same commit results in the following failure:
```
File "/path/to/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/path/to/bitcoin/test/functional/wallet_keypool.py", line 114, in run_test
assert_equal(kp_size_before, kp_size_after)
File "/path/to/bitcoin/test/functional/test_framework/util.py", line 57, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not([18, 24] == [19, 24])
```
This happens because we pass `nIndex` (which is a class member) into `GetReservedDestination` and since it's passed by reference we get an updated value back, so `nIndex` won't be equal `-1` anymore, no matter if the function failed or succeeded. This means that `ReturnDestination` (called by dtor of `ReserveDestination`) will try to return something we did not actually reserve.
The fix is to simply use a temporary variable instead of a class member and only update `nIndex` when `op_address` actually has value, basically do it the same way we do for other class members (`address` and `fInternal`) already.
ACKs for top commit:
achow101:
ACK e073f1dfda7a2a2cb2be9fe2a1d576f122596021
josibake:
ACK e073f1dfda
Tree-SHA512: 1128288a60dd4d8f306ef6f7ac66cdfeae3c9cc35c66ecada2d78fa61ac759f2a757b70fc3976ba8b5081200942b58dfabc184c01ccf911af40ba8c145344651
Skipped tests:
- feature_filelock
- wallet_descriptors
- interface_bitcoin_cli
And all functional tests which explitely specify they are using non-descriptor wallets
24d2d3341d07509ad3f37bb6f130446ad20ac807 QA: wallet_multiwallet: Check that recursive symlink directory and wallet.dat loops are ignored (Luke Dashjr)
69f59af54d15ee9800d5df86bcdb0e962c71e7c3 Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks (Luke Dashjr)
Pull request description:
Previously, an exception would be thrown, which could kill the node in some circumstances.
Includes test changes to cause failure.
Review with `?w=1`
ACKs for top commit:
hebasto:
re-ACK 24d2d3341d07509ad3f37bb6f130446ad20ac807, rebased only since my [previous](https://github.com/bitcoin/bitcoin/pull/19502#pullrequestreview-520552944) review.
promag:
Tested ACK 24d2d3341d07509ad3f37bb6f130446ad20ac807, test change fails on master.
meshcollider:
utACK 24d2d3341d07509ad3f37bb6f130446ad20ac807
Tree-SHA512: f701f81b3aa3d3e15cee52ac9e7c31a73c0d8166e56bf077235294507cbcee099829fedc432a1c4b6d8780885f4e37897b44b980b08125771de3c849c000499e
c4a29d0a90b821c443c10891d9326c534d15cf97 Update wallet_multiwallet.py for descriptor and sqlite wallets (Russell Yanofsky)
310b0fde04639b7446efd5c1d2701caa4b991b86 Run dumpwallet for legacy wallets only in wallet_backup.py (Andrew Chow)
6c6639ac9f6e1677da066cf809f9e3fa4d2e7c32 Include sqlite3 in documentation (Andrew Chow)
f023b7cac0eb16d3c1bf40f1f7898b290de4cc73 wallet: Enforce sqlite serialized threading mode (Andrew Chow)
6173269866306058fcb1cc825b9eb681838678ca Set and check the sqlite user version (Andrew Chow)
9d3d2d263c331e3c77b8f0d01ecc9fea0407dd17 Use network magic as sqlite wallet application ID (Andrew Chow)
9af5de3798c49f86f27bb79396e075fb8c1b2381 Use SQLite for descriptor wallets (Andrew Chow)
9b78f3ce8ed1867c37f6b9fff98f74582d44b789 walletutil: Wallets can also be sqlite (Andrew Chow)
ac38a87225be0f1103ff9629d63980550d2f372b Determine wallet file type based on file magic (Andrew Chow)
6045f77003f167bee9a85e2d53f8fc6ff2e297d8 Implement SQLiteDatabase::MakeBatch (Andrew Chow)
727e6b2a4ee5abb7f2dcbc9f7778291908dc28ad Implement SQLiteDatabase::Verify (Andrew Chow)
b4df8fdb19fcded7e6d491ecf0b705cac0ec76a1 Implement SQLiteDatabase::Rewrite (Andrew Chow)
010e3659069e6f97dd7b24483f50ed71042b84b0 Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort (Andrew Chow)
ac5c1617e7f4273daf24c24da1f6bc5ef5ab2d2b Implement SQLiteDatabase::Backup (Andrew Chow)
f6f9cd6a64842ef23777312f2465e826ca04b886 Implement SQLiteBatch::StartCursor, ReadAtCursor, and CloseCursor (Andrew Chow)
bf90e033f4fe86cfb90492c7e0962278ea3a146d Implement SQLiteBatch::ReadKey, WriteKey, EraseKey, and HasKey (Andrew Chow)
7aa45620e2f2178145a2eca58ccbab3cecff08fb Add SetupSQLStatements (Andrew Chow)
6636a2608a4e5906ee8092d5731595542261e0ad Implement SQLiteBatch::Close (Andrew Chow)
93825352a36456283bf87e39b5888363ee242f21 Implement SQLiteDatabase::Close (Andrew Chow)
a0de83372be83f59015cd3d61af2303b74fb64b5 Implement SQLiteDatabase::Open (Andrew Chow)
3bfa0fe1259280f8c32b41a798c9453b73f89b02 Initialize and Shutdown sqlite3 globals (Andrew Chow)
5a488b3d77326a0d957c1233493061da1b6ec207 Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch (Andrew Chow)
ca8b7e04ab89f99075b093fa248919fd10acbdf7 Implement SQLiteDatabaseVersion (Andrew Chow)
7577b6e1c88a1a7b45ecf5c7f1735bae6f5a82bf Add SQLiteDatabase and SQLiteBatch dummy classes (Andrew Chow)
e87df8258090138d5c22ac46b8602b618620e8a1 Add sqlite to travis and depends (Andrew Chow)
54729f3f4e6765dfded590af5fb28c88331685f8 Add libsqlite3 (Andrew Chow)
Pull request description:
This PR adds a new class `SQLiteDatabase` which is a subclass of `WalletDatabase`. This provides access to a SQLite database that is used to store the wallet records. To keep compatibility with BDB and to complexity of the change down, we don't make use of many SQLite's features. We use it strictly as a key-value store. We create a table `main` which has two columns, `key` and `value` both with the type `blob`.
For new descriptor wallets, we will create a `SQLiteDatabase` instead of a `BerkeleyDatabase`. There is no requirement that all SQLite wallets are descriptor wallets, nor is there a requirement that all descriptor wallets be SQLite wallets. This allows for existing descriptor wallets to work as well as keeping open the option to migrate existing wallets to SQLite.
We keep the name `wallet.dat` for SQLite wallets. We are able to determine which database type to use by searching for specific magic bytes in the `wallet.dat` file. SQLite begins it's files with a null terminated string `SQLite format 3`. BDB has `0x00053162` at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is a `wallet.dat` file that we want to open, we check for the magic bytes to determine which database system to use.
I decided to keep the `wallet.dat` naming to keep things like backup script to continue to function as they won't need to be modified to look for a different file name. It also simplifies a couple of things in the implementation and the tests as `wallet.dat` is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which creates `wallet.sqlite` files instead, but I find that this direction is easier.
ACKs for top commit:
Sjors:
re-utACK c4a29d0a90b821c443c10891d9326c534d15cf97
promag:
Tested ACK c4a29d0a90b821c443c10891d9326c534d15cf97.
fjahr:
reACK c4a29d0a90b821c443c10891d9326c534d15cf97
S3RK:
Re-review ACK c4a29d0a90b821c443c10891d9326c534d15cf97
meshcollider:
re-utACK c4a29d0a90b821c443c10891d9326c534d15cf97
hebasto:
re-ACK c4a29d0a90b821c443c10891d9326c534d15cf97, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/19077#pullrequestreview-507743699) review, verified with `git range-diff master d18892dcc c4a29d0a9`.
ryanofsky:
Code review ACK c4a29d0a90b821c443c10891d9326c534d15cf97. I am honestly confused about reasons for locking into `wallet.dat` again when it's so easy now to use a clean format. I assume I'm just very dense, or there's some unstated reason, because the only thing that's been brought up are unrealistic compatibility scenarios (all require actively creating a wallet with non-default descriptor+sqlite option, then trying to using the descriptor+sqlite wallets with old software or scripts and ignoring the results) that we didn't pay attention to with previous PRs like #11687, which did not require any active interfaction.
jonatack:
ACK c4a29d0a90b821c443c10891d9326c534d15cf97, debug builds and test runs after rebase to latest master @ c2c4dbaebd9, some manual testing creating, using, unloading and reloading a few different new sqlite descriptor wallets over several node restarts/shutdowns.
Tree-SHA512: 19145732e5001484947352d3175a660b5102bc6e833f227a55bd41b9b2f4d92737bbed7cead64b75b509decf9e1408cd81c185ab1fb4b90561aee427c4f9751c
b79dbe86a9886b3539512f6c35205f4c5454a952 test: add feature_rbf.py --descriptors to test_runner.py (Sebastian Falbesoner)
166f8ec28e48aa4c0cd94544909c488df553d8ef test: always rescan after importing private keys in `init_wallet` helper (Sebastian Falbesoner)
Pull request description:
The functional test feature_rbf.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`). This is due to the fact that in this case, a call to the helper `init_wallet`
111c3e06b3/test/functional/test_framework/test_framework.py (L428-L434)
creates a wallet without rescanning the blockchain; the test framework maps the importprivkey RPC calls to the importdescriptors RPC without rescanning by default (timestamp='now'). Fix this by always calling with `rescan=True`, which calls importdescriptors with timestamp=0. Also add `feature_rbf.py --descriptors` to the list of the test runner's calls.
Fixes#23563.
ACKs for top commit:
mjdietzx:
ACK b79dbe86a9886b3539512f6c35205f4c5454a952
Tree-SHA512: a3f3f7a4077066e3c910919d3b5e04bc6b580c1e0a06e9a2fc258950eaea5e59c0f805c8f00432aea722609f2f7e41eebfab653471b76729c5a316825a3d8c86
7411876c75471a45ad40c38c668db3a4b9413fb9 Ensure a legacy wallet for BDB format check (Andrew Chow)
586640381a2c379ce3d6366b1b4534ccc4e8ccf2 Skip --descriptor tests if sqlite is not compiled (Andrew Chow)
Pull request description:
#20156 allows sqlite to not be compiled by configuring `--without-sqlite`. However doing so and then running the test runner will result in all of the `--descriptor` tests to fail. We should be skipping those tests if sqlite was not compiled.
ACKs for top commit:
practicalswift:
ACK 7411876c75471a45ad40c38c668db3a4b9413fb9: patch looks correct
Sjors:
tACK 7411876c75471a45ad40c38c668db3a4b9413fb9
ryanofsky:
Code review ACK 7411876c75471a45ad40c38c668db3a4b9413fb9
hebasto:
ACK 7411876c75471a45ad40c38c668db3a4b9413fb9, tested on Linux Mint 20 (x86_64), tests pass for binaries compiled with:
Tree-SHA512: 1d635a66d2b7bb865300144dfefcfdaf86133aaaa020c8f440a471476ac1205d32f2df704906ce6c2ea48ddf791c3c95055f6291340b4f7b353c1b02cab5cabe
624bab00dd2cc8e2ebd77dc0a669bc8d507c3721 test: add coverage for getwalletinfo format field (Jon Atack)
5e737a009234cbd7cf53748d3d28a2da5221192f rpc, wallet: Expose database format in getwalletinfo (João Barbosa)
Pull request description:
Support for sqlite based wallets was added in #19077. This PR adds the `format` key in `getwalletinfo` response, that can be `bdb` or `sqlite`.
ACKs for top commit:
jonatack:
Tested ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721
laanwj:
Code review ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721.
MarcoFalke:
doesn't hurt ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721
hebasto:
ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721, tested on Linux Mint 20 (x86_64).
meshcollider:
utACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721
Tree-SHA512: a81f8530f040f6381d33e073a65f281993eccfa717424ab6e651c1203cbaf27794dcb7175570459e7fdaa211565bc060d0a3ecbe70d2b6f9c49b8d5071e4441c
fa4074b395a47c54069bd9f598244701505ff11d Show name, format and if uses descriptors in bitcoin-wallet tool (Jonas Schnelli)
Pull request description:
ACKs for top commit:
MarcoFalke:
ACK fa4074b395a47c54069bd9f598244701505ff11d
jonatack:
re-ACK fa4074b395a47c54069bd9f598244701505ff11d
Tree-SHA512: cf6ee96ff21532fc4b0ba7a0fdfdc1fa485c9b1495447350fe65cd0bd919e0e0280613933265cdee069b8c29ccf015ac374535a70cac3d4fb89f4d08b3a03519
faa26d374425f52e03efff3a575c391b7862abe5 test: Remove RPCOverloadWrapper boilerplate (MarcoFalke)
Pull request description:
There are too many wrappers in test_node already, so at least the code that implements the wrappers should be as minimal as possible.
ACKs for top commit:
laanwj:
code review ACK faa26d374425f52e03efff3a575c391b7862abe5
Tree-SHA512: 94e593907de22187524e2445afb3101e40b3b599d4b4015aa8c6ca902d7586ff9daf520828759029d199a3af79e61b96b490a822a5a193ac7bf946beacb11a24
538be4219ae7e65862e4aff540af88c9421e6061 wallet: fix importdescriptor silent fail (Ivan Metlushko)
Pull request description:
Currently `importdescriptor` command will successfully import a descriptor with hardened derivations into a watch-only wallet while silently failing to expand the descriptor to fill the cache. This leads to a broken wallet state and failure to load such wallet due to missing cache on subsequent restart.
ACKs for top commit:
laanwj:
Code review ACK 538be4219ae7e65862e4aff540af88c9421e6061
achow101:
ACK 538be4219ae7e65862e4aff540af88c9421e6061
meshcollider:
utACK 538be4219ae7e65862e4aff540af88c9421e6061
Tree-SHA512: 4bdd0ab4437d55b3f1a79c3a300a0b186089155c020fe220a73d0cce274de47d90371d88918d39fd795f9fccf8db328f1e322d29a6062f9ce94a1c254398f004
bd93fc9945bfd4be117990c5d861f61ddd451f96 Fix change detection of imported internal descriptors (Andrew Chow)
Pull request description:
Import internal descriptors were having address book entries added which meant they would be detected as non-change. Fix this and add a test for it.
ACKs for top commit:
laanwj:
Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96
meshcollider:
utACK bd93fc9945bfd4be117990c5d861f61ddd451f96
promag:
Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96.
Tree-SHA512: 8fa9e364be317627ec171eedffdb505976c0e7f1e55bc7e8cfdffa3aeea5db24d231f55166602cd0e97a5ba621acc871de0a765c75d0c65678f83e93c3b657c5
7b2b06dfe3061b5ab4a283245930e2f7773eb3ef tests: Add missing sync_all to wallet_importdescriptors.py (Andrew Chow)
Pull request description:
node1 will sometimes do sendtoaddress before it has received a funding transaction which will cause the test to fail. sync_all to ensure it gets the transaction first.
Fixes#18800
ACKs for top commit:
instagibbs:
ACK 7b2b06d The wallet endpoint right after is indeed node 1.
Tree-SHA512: b610a771d062b5f955cd70b34337577a1ab8dacbf4be20aa74e1e8234495b0be9faff138eb1713f29decb5574446e0583e221bc2c9a6eea13611b422ea3a296a
