Age | Commit message (Collapse) | Author |
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
to 65 non-witness bytes
b2aa9e85289fc654106a890c35935e9c76c411fb Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders)
8c5b3646b5afe8a61f5c66478d8e11f0d2ce5108 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders)
Pull request description:
Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2)
was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
Related mailing list discussions here:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html
And a couple years earlier:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html
ACKs for top commit:
achow101:
reACK b2aa9e85289fc654106a890c35935e9c76c411fb
glozow:
reACK b2aa9e85289fc654106a890c35935e9c76c411fb
pablomartin4btc:
re-ACK https://github.com/bitcoin/bitcoin/commit/b2aa9e85289fc654106a890c35935e9c76c411fb
jonatack:
ACK b2aa9e85289fc654106a890c35935e9c76c411fb with some suggestions
Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad
|
|
can't run unsigned arm64 binaries; self-sign when needed
dc12f2e212dfacbe238cf68eb454b9ec71169bbc test: improve error msg on previous release tarball extraction failure (kdmukai)
7121fd8fa7de50ff67157f81f9e0f267b9795dbb test: self-sign previous release binaries for arm64 macOS (kdmukai)
Pull request description:
## The Problem
If you run `test/get_previous_releases.py -b` on an M1 or M2 mac, you'll get an unsigned v23.0 binary in the arm64 tarball. macOS [sets stricter requirements on ARM binaries](https://news.ycombinator.com/item?id=26996578) so the unsigned arm64 binary is apparently completely unusable without being signed/notarized(?).
This means that any test that depends on a previous release (e.g. `wallet_backwards_compatibility.py`) will fail because the v23.0 node cannot launch:
```
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes
node.wait_for_rpc_connection()
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status -9 during initialization
```
This can also be confirmed by downloading bitcoin-23.0-arm64-apple-darwin.tar.gz (https://bitcoincore.org/bin/bitcoin-core-23.0/) and trying to run any of the binaries manually on an M1 or M2 mac.
## Solution in this PR
(UPDATED) Per @ hebasto, we can self-sign the arm64 binaries. This PR checks each binary in the previous release's "bin/" and verifies if the arm64 binary is signed. If not, attempt to self-sign and confirm success.
(note: an earlier version of this PR downloaded the x86_64 binary as a workaround but this approach has been discarded)
## Longer term solution
If possible, produce signed arm64 binaries in a future v23.x tarball?
Note that this same problem affects the new v24.0.1 arm64 tarball so perhaps a signed v24.x.x tarball would also be ideal?
That being said, this PR will check all current and future arm64 binaries and self-sign as needed, so perhaps we need not worry about pre-signing the tarball binaries. And I did test a version of `get_previous_releases.py` that includes the new v24.0.1 binaries and it successfully self-signed both v23.0 and v24.0.1, as expected.
## Further info:
Somewhat related to: https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1265164753
And @ fanquake noted on IRC that you can confirm which binaries are or are not signed via:
```
$ codesign -v -d bitcoin-qt
bitcoin-qt: code object is not signed at all
```
ACKs for top commit:
hebasto:
ACK dc12f2e212dfacbe238cf68eb454b9ec71169bbc
Tree-SHA512: 644895f8e97f5ffb3c4754c1db2c48abd77fa100c2058e3c896af04806596fc2b9c807a3f3a2add5be53301ad40ca2b8171585bd254e691f6eb38714d938396b
|
|
option properly
d3a84347e812b746a555e895594c595982815081 ci: remove --prefix from msan job (fanquake)
574e50addf8c65a3a9e0f2d8e933c147d1e93932 ci: Use `CONFIG_SITE` variable and `--prefix` option properly (Hennadii Stepanov)
Pull request description:
When running CI scripts locally, they attempt to use a `$DEPENDS_DIR/$HOST` directory even `NO_DEPENDS=1` is provided.
This PR fixes this broken behavior.
Top commit has no ACKs.
Tree-SHA512: 5e83b921763e6d463e520bbee2ed1599e9f4de36668d19b23dd9d2d7e4441c415e275f588c585b72cadda8bfab5a938979acc1ee4963230aa47081785c741e98
|
|
97115de1839c69d63cae6ea772c17905739bf0ae doc: Refactor/Format getrawtransaction RPC docs and add ScriptPubKeyDoc function (Douglas Chimento)
Pull request description:
Added `ScriptPubKeyDoc` function
ACKs for top commit:
MarcoFalke:
ACK 97115de1839c69d63cae6ea772c17905739bf0ae
kristapsk:
cr utACK 97115de1839c69d63cae6ea772c17905739bf0ae
Tree-SHA512: 1371375986177862e8c99923eb7f1800fef8da7a7ac9f0ec9037bf5b23681c3348d5afe913aab7457f029ee1774d160ac10d7f57238500a03c6385cc0c7013fc
|
|
istream_iterator
bb5ea1d9a954b7b9f443ee8fbbb04549cd0b08a7 qt: Load PSBTs using istreambuf_iterator rather than istream_iterator (Andrew Chow)
Pull request description:
`istream_iterator` eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters. `istreambuf_iterator` is the correct thing to use here.
This is a regression in 24.0. https://github.com/bitcoin/bitcoin/pull/25001 accidentally changed the original `istreambuf_iterator` to `istream_iterator`.
ACKs for top commit:
furszy:
Tested ACK bb5ea1d9
MarcoFalke:
review ACK bb5ea1d9a954b7b9f443ee8fbbb04549cd0b08a7 🍇
Tree-SHA512: 35d90eee3efdcb6a360af69ac1727f9f2837ea621297196de3136299f5de6d9975df4e425e1fc5b8813c1ddb2a4d60c3969e1d5d968953a4628ca45e37d3bf05
|
|
by 18-35% via `keypoolrefill()`
31fdc54dba92fe9d7d8289f1e4380082838a74a9 test: speed up wallet_fundrawtransaction.py and wallet_sendall.py (kdmukai)
Pull request description:
## Problem
`wallet_fundrawtransaction.py` and `wallet_sendall.py` are the two slowest functional tests *when running without a RAM disk*.
```
# M1 MacBook Pro timings
wallet_fundrawtransaction.py --descriptors | ✓ Passed | 55 s
wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 381 s
wallet_sendall.py --descriptors | ✓ Passed | 43 s
wallet_sendall.py --legacy-wallet | ✓ Passed | 327 s
```
In each case, the majority of the time is spent iterating through 1500 to 1600 `getnewaddress()` calls. This is particularly slow in the `--legacy-wallet` runs.
see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_fundrawtransaction.py#L986-L987
see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L324
## Solution
Pre-fill the keypool before iterating through those `getnewaddress()` calls.
With this change, the execution time drops to:
```
wallet_fundrawtransaction.py --descriptors | ✓ Passed | 52 s # -3s diff
wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 291 s # -90s diff
wallet_sendall.py --descriptors | ✓ Passed | 27 s # -16s diff
wallet_sendall.py --legacy-wallet | ✓ Passed | 228 s # -99s diff
```
---
Tagging @ Sjors as he had encouraged me to take a look at speeding up the tests.
ACKs for top commit:
achow101:
ACK 31fdc54dba92fe9d7d8289f1e4380082838a74a9
Tree-SHA512: e8dd89323551779832a407d068977c827c09dff55c1079d3c19aab39fcce6957df22b1da797ed7aa3bc2f6dd22fdf9e6f5e1a9a0200fdb16ed6042fc5f6dd992
|
|
17554efb6095f6ec273e52568efe1678253cb7c0 test: prefer sqlite for wallet tests (S3RK)
8e0fabaabf8f45b82256c1934c2507d03351acaa test: make wallet_migration.py pass with both wallet flags (S3RK)
Pull request description:
Fixes #26511
ACKs for top commit:
MarcoFalke:
review ACK 17554efb6095f6ec273e52568efe1678253cb7c0
achow101:
ACK 17554efb6095f6ec273e52568efe1678253cb7c0
Tree-SHA512: 97cae275998f07032feffe1b533d4747b8ff03c3c1fb830af69ee38cadb75fd58532956f66f79c0d275b00620ce53b0b5240f885e4f29b8bd4d0b6e6cbc683fa
|
|
|
|
2c07cfacd1745844a1d3c57f2e8617549b9815d7 gui: bumpfee signer support (Sjors Provoost)
7e02a3329797211ed5d35e5f5e7b919c099b78ba rpc: bumpfee signer support (Sjors Provoost)
304ece994504220c355577170409b9200941f2af rpc: document bools in FillPSBT() calls (Sjors Provoost)
Pull request description:
The `bumpfee` RPC call and GUI fee bump interface now work with an external signer.
ACKs for top commit:
achow101:
ACK 2c07cfacd1745844a1d3c57f2e8617549b9815d7
furszy:
code review ACK 2c07cfac
jarolrod:
tACK 2c07cfa
Tree-SHA512: 0c7b931f76fac67c9e33b9b935f29af6f69ac67a5ffcc586ed2f1676feac427735b1d971723b29ef332bb6fb5762949598ebbf728587e8f0ded95a9bfbb3e7a4
|
|
1b228497fa729c512a15bdfa80f61a610abfe8a5 qt: Drop no longer used `SplashScreen::finish()` slot (Hennadii Stepanov)
10811afff40efed1fda7eecab89884eaadd7146c qt: Drop no longer used `BitcoinApplication::splashFinished()` signal (Hennadii Stepanov)
5299cfe371ddad806b1c4538d17cde069e25b1c1 qt: Delete splash screen widget explicitly (Hennadii Stepanov)
Pull request description:
Fixes bitcoin-core/gui#604.
Fixes bitcoin/bitcoin#25146.
Fixes bitcoin/bitcoin#26340.
`SplashScreen::deleteLater()` [does not guarantee](https://doc.qt.io/qt-5/qobject.html#deleteLater) deletion of the `m_splash` object prior to the wallet context deletion. If the latter happens first, the [segfault](https://github.com/bitcoin-core/gui/issues/604#issuecomment-1133907013) follows.
ACKs for top commit:
dooglus:
ACK https://github.com/bitcoin-core/gui/commit/1b228497fa729c512a15bdfa80f61a610abfe8a5
furszy:
ACK 1b228497
john-moffett:
ACK 1b228497fa729c512a15bdfa80f61a610abfe8a5
Tree-SHA512: bb01d0bf2051f5b184dc415c4f5d32dfb7b8bd772feff7ec7754ded4c6482de27f004b9712df7d53c5ee82e153f48aef4372e4a49d7bcbbb137f73e9b4947962
|
|
|
|
a4defcdd57918f3693902cc40a54d0e64d716d50 test, lint: add `crypted` to `ignore-words` (brunoerg)
Pull request description:
Fixes #26719
"Crypted" is used in some comments at `walletload_tests` because it refers to `DBKeys::CRYPTED_KEY`, it's not necessary
a mistake.
Obs: I can change the approach (changing `walletload_tests` comments to use `encrypted` word instead of adding it to the `ignore_words`) if reviewers think it makes more sense.
ACKs for top commit:
achow101:
ACK a4defcdd57918f3693902cc40a54d0e64d716d50
Tree-SHA512: 49f38eed30ffb0fda4e792566591c3455629379619eb9a5c4240c5b00e14cd27ba1faa36337192233752e642f0998373b86fcb8ca586508bbf15900d68b17950
|
|
|
|
|
|
|
|
|
|
Since the original fix was set to be a "reasonable" transaction
to reduce allocations and the true motivation later revealed,
it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage
of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn
a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2) was the route taken.
It would not allow an "empty" OP_RETURN
but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
|
|
disabled should throw an error
ec63a4892e67fdc6c0c464e6d4cffcd3eb48941c test: call `keypoolrefill` with private keys disabled should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
https://github.com/bitcoin/bitcoin/blob/cb32328d1b80d0ccd6eb9532bd8fe4e0a4de385e/src/wallet/rpc/addresses.cpp#L332-L334
ACKs for top commit:
aureleoules:
ACK ec63a4892e67fdc6c0c464e6d4cffcd3eb48941c
Tree-SHA512: b5deda8981ff472f290e6e16c8723a58e02cbe099afd1f672c099f4add0a1d9b192b11a2c3f0e11b96794671f6b9efa75812b7a174248d7c58d7fd7d3310e7b9
|
|
CNodeStateStats
6fefd49527fa0ed9535e54f2a3e76fe2599b2350 rpc: Require NodeStateStats object in getpeerinfo (Martin Zumsande)
Pull request description:
The objects `CNode`, `CNodeState` and `Peer` store different info about a peer - `InitializeNode()` and `FinalizeNode()` make sure that for the duration of a connection, we should always have one of each for a peer.
Therefore, there is no situation in which, as part of getpeerinfo RPC, `GetNodeStateStats()` (which requires a `CNodeState` and a `Peer` entry for a `NodeId` to succeed) could fail for a legitimate reason while the peer is connected - this can only happen if there is a race condition between peer disconnection and the `getpeerinfo` processing (see also a more detailed description of this in https://github.com/bitcoin/bitcoin/pull/26457#pullrequestreview-1181641835).
But in this case I think it's better to just not include the newly disconnected peer in the response instead of returning just parts of its data.
An earlier version of this PR also made the affected `CNodeStateStats` fields non-optional (see https://github.com/mzumsande/bitcoin/commit/5f900e27d0e5ceaa3b800a2eb5a8e8ff6bccad3b). Since this conflicts with #25923 and should be a separate discussion, I removed that commit from this PR.
ACKs for top commit:
dergoegge:
Approach ACK 6fefd49527fa0ed9535e54f2a3e76fe2599b2350
MarcoFalke:
review ACK 6fefd49527fa0ed9535e54f2a3e76fe2599b2350 👒
Tree-SHA512: 89c8f7318df4634c1630415de9c8350e6dc2d14d9d07e039e5b180c51bfd3ee2ce99eeac4f9f858af7de846f7a6b48fcae96ebac08495b30e431a5d2d4660532
|
|
36c201feb74bbb87d22bd956373dbbb9c47fb7e7 remove CBlockIndex copy construction (James O'Beirne)
Pull request description:
Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer members (e.g. pprev).
(See also https://github.com/bitcoin/bitcoin/pull/24008#discussion_r891949166)
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
ACKs for top commit:
ajtowns:
ACK 36c201feb74bbb87d22bd956373dbbb9c47fb7e7 - code review only
MarcoFalke:
re-ACK 36c201feb74bbb87d22bd956373dbbb9c47fb7e7 🏻
Tree-SHA512: b1cf9a1cb992464a4377dad609713eea63cc099435df374e4553bfe62d362a4eb5e3c6c6649177832f38c0905b23841caf9d62196cef8e3084bfea0bfc26374b
|
|
`--enable-debug`
1647a11f39cfa2c2847c52100bb69cfdfc63723f tests: Reorder longer running tests in test_runner (Andrew Chow)
ff6c9fe02743c9628e49a504b6b879d687c7390f tests: Whitelist test p2p connection in rpc_packages (Andrew Chow)
8c20796aacbbf5261e1922d45fc8afe75f54fefb tests: Use waitfornewblock for work queue test in interface_rpc (Andrew Chow)
6c872d5e656a7117bbdf19a0220572b93de16f31 tests: Initialize sigops draining script with bytes in feature_taproot (Andrew Chow)
544cbf776cf25d90ea4b96d92e7ee6e316576038 tests: Use batched RPC in feature_fee_estimation (Andrew Chow)
4ad7272f8b24843582e05e7dfc15f1e058e1a0f3 tests: reduce number of generated blocks for wallet_import_rescan (Andrew Chow)
Pull request description:
When configured with `--enable-debug`, many tests become dramatically slower. These slow downs are particularly noticed in tests that generate a lot of blocks in separate calls, make a lot of RPC calls, or send a lot of data from the test framework's P2P connection. This PR aims to improve the runtime of some of the slower tests and improve the overall runtime of the test runner. This has improved the runtime of the test runner from ~400s to ~140s on my computer.
The slowest test by far was `wallet_import_rescan.py`. This was taking ~320s. Most of that time was spent waiting for blocks to be mined and then synced to the other nodes. It was generating a new block for every new transaction it was creating in a setup loop. However it is not necessary to have one tx per block. By mining a block only every 10 txs, the runtime is improved to ~61s.
The second slowest test was `feature_fee_estimation.py`. This test spends most of its time waiting for RPCs to respond. I was able to improve its runtime by batching RPC requests. This has improved the runtime from ~201s to ~140s.
In `feature_taproot.py`, the test was constructing a Python `CScript` using a very large list of `OP_CHECKSIG`s. The constructor for the Python implementation of `CScript` was iterating this list in order to create a `bytes` from it even though a `bytes` could be created from it without iterating. By making the `bytes` before passing it into the constructor, we are able to improve this test's runtime from ~131s to ~106s.
Although `interface_rpc.py` was not typically a slow test, I found that it would occasionally have a super long runtime. It typically takes ~7s, but I have observed it taking >400s to run on occasion. This longer runtime occurs more often when `--enable-debug`. This long runtime was caused by the "exceeding work queue" test which is really just trying to trigger a race condition. In this test, it would create a few threads and try an RPC in a loop in the hopes that eventually one of the RPCs would be added to the work queue while another was processing. It used `getrpcinfo` for this, but this function is fairly fast. I believe what was happening was that with `--enable-debug`, all of the code for receiving the RPC would often take longer to run than the RPC itself, so the majority of the requests would succeed, until we got lucky after 10's of thousands of requests. By changing this to use a slow RPC, the race condition can be triggered more reliably, and much sooner as well. I've used `waitfornewblock` with a 500ms timeout. This improves the runtime to ~3s consistently.
The last test I've changed was `rpc_packages.py`. This test was one of the higher runtime variability tests. The main source of this variation appears to be waiting for the test node to relay a transaction to the test framework's P2P connection. By whitelisting that peer, the variability is reduced to nearly 0.
Lastly, I've reordered the tests in `test_runner.py` to account for the slower runtimes when configured with `--enable-debug`. Some of the slow tests I've looked at were listed as being fast which was causing overall `test_runner.py` runtime to be extended. This change makes the test runner's runtime be bounded by the slowest test (currently `feature_fee_estimation.py` with my usual config (`-j 60`).
ACKs for top commit:
willcl-ark:
ACK 1647a11
Tree-SHA512: 529e0da4bc51f12c78a40d6d70b3a492b97723c96a3526148c46943d923c118737b32d2aec23d246392e50ab48013891ef19fe6205bf538b61b70d4f16a203eb
|
|
2b77a33e5b91a2e54c5e99b11bd775807ade024d test: Improve `check-doc.py` pattern (Hennadii Stepanov)
Pull request description:
On master (cb32328d1b80d0ccd6eb9532bd8fe4e0a4de385e):
```
$ ./test/lint/check-doc.py
Args used : 158
Args documented : 219
Args undocumented: 0
set()
Args unknown : 61
{'-stopatheight', '-maxtipage', '-maxreceivebuffer', '-txconfirmtarget', '-maxconnections', '-maxsigcachesize', '-peertimeout', '-limitancestorsize', '-output-csv', '-blockmaxweight', '-par', '-rpcclienttimeout', '-dbcrashratio', '-zmqpubsequence', '-zmqpubhashtxhwm', '-zmqpubrawblock', '-dbbatchsize', '-zmqpubrawtxhwm', '-includeconf', '-checkblocks', '-limitancestorcount', '-zmqpubrawtx', '-checklevel', '-checkmempool', '-rpcthreads', '-rpcworkqueue', '-zmqpubsequencehwm', '-zmqpubrawblockhwm', '-rpcservertimeout', '-testnet', '-zmqpubhashtx', '-signet', '-rpcwaittimeout', '-limitdescendantcount', '-output-json', '-maxmempool', '-mocktime', '-datacarriersize', '-rpcport', '-dbcache', '-zmqpubhashblockhwm', '-mempoolexpiry', '-settings', '-min-time', '-maxtimeadjustment', '-bytespersigop', '-blockversion', '-limitdescendantsize', '-maxorphantx', '-rpccookiefile', '-rpcserialversion', '-bantime', '-blockreconstructionextratxn', '-checkaddrman', '-debuglogfile', '-pid', '-dblogsize', '-timeout', '-zmqpubhashblock', '-maxsendbuffer', '-regtest'}
```
With this PR:
```
$ ./test/lint/check-doc.py
Args used : 208
Args documented : 219
Args undocumented: 0
set()
Args unknown : 11
{'-zmqpubrawblock', '-zmqpubhashblockhwm', '-zmqpubsequencehwm', '-zmqpubrawtx', '-zmqpubhashblock', '-zmqpubhashtx', '-includeconf', '-zmqpubhashtxhwm', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm', '-zmqpubsequence'}
```
ACKs for top commit:
vincenzopalazzo:
ACK https://github.com/bitcoin/bitcoin/pull/26717/commits/2b77a33e5b91a2e54c5e99b11bd775807ade024d
Tree-SHA512: 6cf4ccc4e8319aad8006ae915f0d25637ac12974fbc1f81808f26b72fbe2649e2b6ff993bc2c1894f81bd6756bff77491b3d56382c034a84fd50325a3c807d8b
|
|
|
|
istream_iterator eats whitespace charactesr which causes parsing
failures for PSBTs that contain the bytes corresponding to those
characters.
|
|
|
|
|
|
headers
48033d43dc30fcee2c9a38c828d2f58d3bc11827 clang-tidy: Fix `performance-for-range-copy` in headers (Hennadii Stepanov)
Pull request description:
Split from bitcoin/bitcoin#26705 as was requested in https://github.com/bitcoin/bitcoin/pull/26705#issuecomment-1353293405.
To test this PR, consider applying a diff as follows:
```diff
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -12,17 +12,9 @@ readability-redundant-declaration,
readability-redundant-string-init,
'
WarningsAsErrors: '
-bugprone-argument-comment,
-bugprone-use-after-move,
-misc-unused-using-decls,
-modernize-use-default-member-init,
-modernize-use-nullptr,
performance-for-range-copy,
-performance-move-const-arg,
-performance-unnecessary-copy-initialization,
-readability-redundant-declaration,
-readability-redundant-string-init,
'
CheckOptions:
- key: performance-move-const-arg.CheckTriviallyCopyableMove
value: false
+HeaderFilterRegex: '.'
```
ACKs for top commit:
MarcoFalke:
review ACK 48033d43dc30fcee2c9a38c828d2f58d3bc11827
Tree-SHA512: eaf7a0e9b4fdc4ce788f78e5675632f3c278fc24bee2434874cbabc3e25ad7059b0c53ab7834908e901872d5afee08acba860542b03454c09fe129be6ad03f09
|
|
adb7dba9de95c166103ac7eaf97d5bd83dc19605 clang-tidy: Fix `modernize-use-nullptr` in headers (Hennadii Stepanov)
Pull request description:
Split from bitcoin/bitcoin#26705 as was requested in https://github.com/bitcoin/bitcoin/pull/26705#issuecomment-1353293405.
To test this PR, consider applying a diff as follows:
```diff
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -12,17 +12,9 @@ readability-redundant-declaration,
readability-redundant-string-init,
'
WarningsAsErrors: '
-bugprone-argument-comment,
-bugprone-use-after-move,
-misc-unused-using-decls,
-modernize-use-default-member-init,
modernize-use-nullptr,
-performance-for-range-copy,
-performance-move-const-arg,
-performance-unnecessary-copy-initialization,
-readability-redundant-declaration,
-readability-redundant-string-init,
'
CheckOptions:
- key: performance-move-const-arg.CheckTriviallyCopyableMove
value: false
+HeaderFilterRegex: '.'
```
ACKs for top commit:
john-moffett:
ACK adb7dba9de95c166103ac7eaf97d5bd83dc19605
Tree-SHA512: 67241fb212d837157a0a26f0d59e7f30a9d270d5b0ebfeb6ad9631e460fc7fba8c9a9dcd4c0520789353f68025a9f090f40f17176472a93cce1411e6d56f930b
|
|
friendly
fafcc9439838b3f084fc054b91bca4b50ee62df5 Make bitcoin-util grind_task tsan friendly (MacroFake)
Pull request description:
While there is no issue with the current code, `libtsan-12.2.1` on my machine does not seem to like it. This is understandable, because the nonce isn't protected by a mutex that the sanitizer can see (only by an atomic, which achieves the same).
Fix this by guarding the nonce by the existing atomic bool, which tsan seems to understand.
ACKs for top commit:
ajtowns:
ACK fafcc9439838b3f084fc054b91bca4b50ee62df5
hebasto:
ACK fafcc9439838b3f084fc054b91bca4b50ee62df5, I have reviewed the code and it looks OK, I agree it can be merged. Confirming that initial bug has been fixed.
Tree-SHA512: 4e67fab5833ec7d91678b85a300368892ee9f7cd89a52cc5e15a7df65b2da813b24eaffd8362d0d8a3c8951e024041d69ebddf25101b11d0a1a62c1208ddc9a5
|
|
add test
564b580bf07742483a140c7c095b896a6d5d6cad test: Introduce MIN_BLOCKS_TO_KEEP constant (Aurèle Oulès)
71d9a7c03b44236c2fea2b74f92a69234d29f717 test: Wallet imports on pruned nodes (Aurèle Oulès)
e6906fcf9e4d5692ead6c9bf5a2e11673315a1f5 rpc: Enable wallet import on pruned nodes (Aurèle Oulès)
Pull request description:
Reopens #16037
I have rebased the PR, addressed the comments of the original PR and added a functional test.
> Before this change importwallet fails if any block is pruned. This PR makes it possible to importwallet if all required blocks aren't pruned. This is possible because the dump format includes key timestamps.
For reviewers:
`python test/functional/wallet_pruning.py --nocleanup` will generate a large blockchain (~700MB) that can be used to manually test wallet imports on a pruned node. Node0 is not pruned, while node1 is.
ACKs for top commit:
kouloumos:
ACK 564b580bf07742483a140c7c095b896a6d5d6cad
achow101:
reACK 564b580bf07742483a140c7c095b896a6d5d6cad
furszy:
ACK 564b580
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/24865/commits/564b580bf07742483a140c7c095b896a6d5d6cad
Tree-SHA512: b345a6c455fcb6581cdaa5f7a55d79e763a55cb08c81d66be5b12794985d79cd51b9b39bdcd0f7ba0a2a2643e9b2ddc49310ff03d16b430df2f74e990800eabf
|
|
a2724808abc8b81260eba2ac5ef2cfd61bb8bf56 doc: add 23.1 release notes (fanquake)
Pull request description:
Same as past releases / https://github.com/bitcoin/bitcoin/pull/26524 etc.
ACKs for top commit:
stickies-v:
ACK a2724808abc8b81260eba2ac5ef2cfd61bb8bf56
Tree-SHA512: e9f7ad72c23c8621e8a98ffa0dc0d08ebe30ad0bc8d23e25fabda5b1a9318ff74c65344821267c6af5a8d94c26c775ce83a67cbe0c4922eac07a4319fd94eb49
|
|
See https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html
|
|
|
|
This does not change behavior of the bitcoin-util binary.
|
|
`readability-redundant-string-init` in headers
c39619eeb4e615eb991143edb99e031dd562d0f6 clang-tidy: Fix `readability-redundant-string-init` in headers (Hennadii Stepanov)
Pull request description:
Split from bitcoin/bitcoin#26705 as was requested in https://github.com/bitcoin/bitcoin/pull/26705#issuecomment-1353293405.
To test this PR, consider applying a diff as follows:
```diff
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -12,17 +12,9 @@ readability-redundant-declaration,
readability-redundant-string-init,
'
WarningsAsErrors: '
-bugprone-argument-comment,
-bugprone-use-after-move,
-misc-unused-using-decls,
-modernize-use-default-member-init,
-modernize-use-nullptr,
-performance-for-range-copy,
-performance-move-const-arg,
-performance-unnecessary-copy-initialization,
-readability-redundant-declaration,
readability-redundant-string-init,
'
CheckOptions:
- key: performance-move-const-arg.CheckTriviallyCopyableMove
value: false
+HeaderFilterRegex: '.'
```
ACKs for top commit:
MarcoFalke:
review ACK c39619eeb4e615eb991143edb99e031dd562d0f6
Tree-SHA512: d7b61be17737f68b8bb40b084cf03f89eae86f4951da2aa000fde0c5245491a01dbb83e5d6e870c6bab4de2dbb5c0eb0dd6613da71592b3a27cf2000a45eaeeb
|
|
See https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-string-init.html
|
|
https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-nullptr.html
|
|
Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer references (e.g. pprev).
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
Delete move constructors and declare the destructor to satisfy the
"rule of 5."
|
|
062e4e9fe9f6121038df5423b6abf83dbee9800f doc: add 22.1 release notes (fanquake)
Pull request description:
Same as past releases / #26524 etc.
Top commit has no ACKs.
Tree-SHA512: e41b1eaff1aacd89260f070650044629de5673020e0e70bdceb0749981403aad380e5595c494fa5ebaaa7c87e0ebea0def5f5bbd433a4b3b810e40c2de6dc448
|
|
feature_assumevalid.py
fa34e5f3d35c81bb4376337f4f76c3d501bca742 test: Avoid intermittent timeout in feature_assumevalid.py (MarcoFalke)
Pull request description:
Currently the test will spin up p2p connections in the beginning, then announce the headers to all nodes, but only send the blocks sequentially. This takes a long time, so when getting to the last node, it will have already timed out, while node1 is busy eating blocks. Example:
```
node2 2022-12-06T19:31:35.419291Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e (1) peer=0
node2 2022-12-06T19:31:35.424784Z [msghand] [net.cpp:2776] [PushMessage] [net] sending getdata (577 bytes) peer=0
[...]
node2 2022-12-06T19:41:35.423257Z [msghand] [net_processing.cpp:5729] [SendMessages] Timeout downloading block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e from peer=0, disconnecting
node1 2022-12-06T19:41:35.438706Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 6575919043306ed309014d0bd79814b4fab8afaa281e026d8cc3a1c4c2336fbc (1748) peer=0
node2 2022-12-06T19:41:35.521253Z [net] [net.cpp:573] [CloseSocketDisconnect] [net] disconnecting peer=0
node2 2022-12-06T19:41:35.630417Z [net] [net_processing.cpp:1532] [FinalizeNode] [net] Cleared nodestate for peer=0
```
Fix this by only spinning up the p2p connection right before they are needed.
ACKs for top commit:
jamesob:
ACK fa34e5f3d35c81bb4376337f4f76c3d501bca742 ([`jamesob/ackr/26651.1.MarcoFalke.test_avoid_intermittent`](https://github.com/jamesob/bitcoin/tree/ackr/26651.1.MarcoFalke.test_avoid_intermittent))
Tree-SHA512: 7a1b114c07dfa30237c8cd8637dd6646c5c2dc2530c9de61db231738fddc800b620c31dc664237e33d35e951cf161f015fda593162efc9d85c5f68c6e37217d4
|
|
Same as past releases / #26524 etc.
|
|
|
|
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: Andreas Kouloumos <kouloumosa@gmail.com>
|
|
perform "mixed" coin selection
89c1491d35389eac0c1fecc59333cdfae3b1bd2c wallet: if only have one output type, don't perform "mixed" coin selection (furszy)
Pull request description:
For wallets that only have one output type, we are currently performing the same
selection process over the same coins twice.
The "mixed coin selection" doesn't add any value to the result
(there is nothing to mix if the available coins struct has only one type).
ACKs for top commit:
achow101:
ACK 89c1491d35389eac0c1fecc59333cdfae3b1bd2c
john-moffett:
ACK 89c1491d35389eac0c1fecc59333cdfae3b1bd2c
kristapsk:
cr utACK 89c1491d35389eac0c1fecc59333cdfae3b1bd2c
Tree-SHA512: 672eaeed3ba911d13fa61a46f719c8fe1ebe4d2dc7d723040e71937c693659411bc99cdbd9f0014e836b70eebeff1b8ca861f4d81d39e6f79f437364a526edbe
|
|
bcb71234065e525e9f3cccceaaab320f2ec3741b test: add add_wallet_options to TestShell (josibake)
Pull request description:
following https://github.com/bitcoin/bitcoin/pull/26480/commits/555519d082fbe5e047595f06d7f301e441bb7149, `TestShell` now always runs with `-disablewallet`. simple fix is to add `add_wallet_options` to `add_options`; looks like testshell was overlooked when adding in the `add_wallet_options` call to the functional tests in #26480
ACKs for top commit:
amitiuttarwar:
ACK bcb71234065e525e9f3cccceaaab320f2ec3741b
Tree-SHA512: db554a8b3c8ff5bd10cab9592b316035a92f86a0a0ae8ff914de9687bbbb6fc2235bdf82c4cd40e4071782f8b6edf91aad372e82ed3b826c9d8ae39dbe3dbf57
|
|
getrawtransaction
f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9 rpc: Return fee and prevout(s) to getrawtransaction (Douglas Chimento)
Pull request description:
Add fee response in BTC to getrawtransaction #23264
### For Reviewers
* Verbose arg is now an int
* Verbose = 2 includes a `fee` field and `prevout`
* [./test/functional/rpc_rawtransaction.py](./test/functional/rpc_rawtransaction.py) contains a new test to validate fields of new verbosity 2 (not the values)
```
bitcoin-cli -chain=test getrawtransaction 9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4 2 000000000000001d442e556146d5f2841d85150c200e8d8b8a4b5005b13878f6
```
```
"in_active_chain": true,
"txid": "9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4",
"hash": "7f23e3f3a0a256ddea1d35ffd43e9afdd67cc68389ef1a804bb20c76abd6863e",
....
"vin": [
{
"txid": "23fc75d6d74f6f97e225839af69ff36a612fe04db58a4414ec4828d1749a05a0",
"vout": 0,
"scriptSig": {
"asm": "",
"hex": ""
},
"prevout": {
"generated": false,
"height": 2099486,
"value": 0.00017764,
"scriptPubKey": {
"asm": "0 7846ce1ced3253d8bd43008db2ca364cc722f5a2",
"hex": "00147846ce1ced3253d8bd43008db2ca364cc722f5a2",
"address": "tb1q0prvu88dxffa302rqzxm9j3kfnrj9adzk49mlp",
"type": "witness_v0_keyhash"
}
},
"sequence": 4294967295
},
...
"fee": 0.00000762
}
```
ACKs for top commit:
achow101:
ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
aureleoules:
ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
hernanmarino:
re ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
pablomartin4btc:
re-tACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
Tree-SHA512: 591fdc285d74fa7803e04ad01c7b70bc20fac6b1369e7bd5b8e2cde9b750ea52d6c70d79225b74bef4f4bbc0fb960877778017184e146119da4a55f9593d1224
|
|
e75d2276324d54a01971afdf531df91748275bd5 Minor fix: Don't directly delete abandoned txes (John Moffett)
Pull request description:
This fully closes bitcoin/bitcoin#12179. Currently, when a user abandons a transaction by clicking "Abandon Transaction" in the context menu, a call is made to remove it from the GUI view:
`model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);`
(The `false` parameter is for `bool showTransaction`)
This behavior is probably unwanted, as the transaction is not actually removed from the wallet and would show up again if the node is restarted.
However, the previous line, `model->wallet().abandonTransaction(hash);`, changes the underlying model and calls `NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);`, which queues a signal that eventually calls back to `updateTransaction`, this time with `showTransaction` set to `true`. This runs on a separate thread, so it gets called *after* the 'subsequent' `updateTransaction`. The transaction gets removed from the GUI and immediately added back.
In a nutshell, `updateTransaction` gets called twice. The first (direct) call deletes the transaction from the GUI. The second (sent via a queued signal) brings it back to the GUI. The first direct call is redundant and unwanted. Worse, if the `abandonTransaction` call fails for any reason, the transaction still gets removed from the GUI. (This is what caused bitcoin#12179. It can still be triggered if, eg., a user clicks "Abandon Transaction" the moment after a new block is found.)
There are no conditions (to my knowledge) where an abandoned transaction should be directly removed from the GUI. If the underlying model changes, the deletion should be reflected anyway by the queued signal to `updateTransaction`.
The behavior is borne out by the QT logs. To reproduce, send a transaction with RBF enabled, then bump the fee, then 'abandon transaction' on the first transaction. The logs will show something like this:
```
2022-11-28T14:48:00Z [qt] GUI: "NotifyTransactionChanged: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 status= 1"
2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
2022-11-28T14:48:00Z [qt] GUI: " inModel=1 Index=381-382 showTransaction=0 derivedStatus=2"
2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
2022-11-28T14:48:00Z [qt] GUI: " inModel=0 Index=381-381 showTransaction=1 derivedStatus=0"
```
Notice the duplicate `updateWallet` calls with different `showTransaction` values.
ACKs for top commit:
hebasto:
ACK e75d2276324d54a01971afdf531df91748275bd5
jarolrod:
tACK e75d2276324d54a01971afdf531df91748275bd5
Tree-SHA512: 00f150f747c2ee1605af861a21d5c3b9773a4a9985e8dab62e48bd32885b1bfa4e8cbf805ad61af77aec9d3ccefaed3f4311a29086aa8c22d55d5326ba68ece6
|
|
all fee has been set
798430d127521d088c081ee625912a704f415990 wallet: Sanity check fee paid cannot be negative (Andrew Chow)
c1a84f108e320bd44c172a4dd3bb486ab777ff69 wallet: Move fee underpayment check to after fee setting (Andrew Chow)
e5daf976d5b064b585029d4bb38d68a8153ea13b wallet: Rename nFeeRet in CreateTransactionInternal to current_fee (Andrew Chow)
Pull request description:
Currently the fee underpayment check occurs right after we calculate what the transaction's fee should be. However the fee paid by the transaction at that time does not always match. Notably, when doing SFFO, the fee paid at that time will almost always be less than the fee required, which then required having a bypass of the underpayment check that results in SFFO payments going through when they should not.
This PR moves the underpayment check to after fees have been finalized so that we always check whether the fee is being underpaid. This removes the exception for SFFO and unifies this behavior for both SFFO and non-SFFO txs.
ACKs for top commit:
S3RK:
Code review ACK 798430d127521d088c081ee625912a704f415990
furszy:
Code review ACK 798430d
glozow:
utACK 798430d127, code looks correct to me
Tree-SHA512: 720e8a3dbdc9937b12ee7881eb2ad58332c9584520da87ef3080e6f9d6220ce8d3bd8b9317b4877e56a229113437340852976db8f64df0d5cc50723fa04b02f0
|
|
parameter specified multiple times
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
|