Intended to make bitcoin command line features more discoverable and allow
installing new multiprocess binaries in libexec/ instead of bin/ so they don't
cause confusion.
Idea and implementation of this were discussed in
https://github.com/bitcoin/bitcoin/issues/30983
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
These functions are just meant to serve the needs of the bitcoin wrapper
executable, and are intentionally not very general purpose so they can be
simple.
3bbdbc0a5e qt, docs: Unify term "clipboard" (Hennadii Stepanov)
Pull request description:
A translator on Transifex noticed:
> The term "system clipboard" appears twice. The term "clipboard" appears 10 times. Perhaps we could standardize on just saying "clipboard"?
This PR addresses this issue.
ACKs for top commit:
davidgumberg:
ACK 3bbdbc0a5e
pablomartin4btc:
ACK 3bbdbc0a5e
Tree-SHA512: 61a100f60890d81122a4b8ce3e2cb7d355c7fb643de3196573f7f9107c6f52fa0b3e7a4f743ce2833e8c67b9cdad3568b761d730fef5c9781f5e1c45252888c4
002b792b9a gui: decouple WalletModel from RPCExecutor (furszy)
Pull request description:
A more comprehensive fix for the issue described in #837.
Since the `WalletModel` class is unavailable when compiling without wallet support
`(-DENABLE_WALLET=0)`, the RPC executor class should not be coupled to it.
This decoupling ensures GUI compatibility with builds that omit wallet support.
This also drops an extra `#ifdef ENABLE_WALLET` block which is always good.
ACKs for top commit:
w0xlt:
Code Review ACK 002b792b9a
pablomartin4btc:
tACK 002b792b9a
BrandonOdiwuor:
tACK 002b792b9a
hebasto:
ACK 002b792b9a, I have reviewed the code and it looks OK.
Tree-SHA512: a8e6b7e9d88dd8e0ff5e2d0de91be2f85fd0559265267d3bf6cae5a37606cf1ab6bc7415d5817a11006008de362f2ca3557ba772b4e1bd9fbef5f564be3b53bb
ab878a7e74 build: simplify *ifaddr handling (fanquake)
Pull request description:
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
`iffaddrs.h` that implements one, but not the other.
ACKs for top commit:
hebasto:
ACK ab878a7e74. Only addressed my [comment](https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079994126) and rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189).
TheCharlatan:
ACK ab878a7e74
Tree-SHA512: 7667305df9fef4728526c7217f85b51e739ec63b38e808da51d6ae65cb6f2696afa5ba82e5a72ed4a7a9b79ffa2402640448af4392587253027122eab7618e30
a58cb3b1c1 qa: sanity check mined block have their coinbase timelocked to height (Antoine Poinsot)
8f2078af6a miner: timelock coinbase transactions (Antoine Poinsot)
788aeebf34 qa: use prev height as nLockTime for coinbase txs created in unit tests (Antoine Poinsot)
c76dbe9b8b qa: timelock coinbase transactions created in fuzz targets (Antoine Poinsot)
9c94069d8b contrib: timelock coinbase transactions in signet miner (Antoine Poinsot)
a5f52cfcc4 qa: timelock coinbase transactions created in functional tests (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the `coinbasetxn` field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.
ACKs for top commit:
Sjors:
Code review ACK a58cb3b1c1
achow101:
ACK a58cb3b1c1
TheCharlatan:
Re-ACK a58cb3b1c1
Tree-SHA512: a2aae009a187eb760d34435f518a895ee76c6b02a667eb030ddf6bd584da6e8eae2737d974dbf81a928d60c07bcb4820f055adc067e18d8819640db0240bb513
1ee698fde2 test: refactor: negate signature-s using libsecp256k1 (Sebastian Falbesoner)
Pull request description:
This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by https://github.com/bitcoin-core/secp256k1/pull/69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see https://github.com/bitcoin-core/secp256k1/pull/408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general for scalars modulo the group order.
ACKs for top commit:
achow101:
ACK 1ee698fde2
laanwj:
Code review ACK 1ee698fde2
w0xlt:
ACK 1ee698fde2
rkrux:
tACK 1ee698fde2
Tree-SHA512: dc36ea1572b538d11ae34e1871f310a1cda8083ffb753e93e7ee9d56e91ebd8ec78d35758dfb700254720914b734ef7a071eeef71b6239f19e1e2fb289fb5435
It is confusing that the chain client flush happens between
StopHTTPServer and StopMapPort. Also, it is unused code. Seems best to
just add it back properly when it is needed again.
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
iffaddrs.h that implements one, but not the other.
4e8ab5e00f crypto: disable ASan for sha256_sse4 with Clang (fanquake)
Pull request description:
This also fails to compile when optimisations are being used, see: https://github.com/bitcoin/bitcoin/issues/31913.
So just disable ASan under any optimisation level.
Closes#31913.
ACKs for top commit:
maflcko:
lgtm ACK 4e8ab5e00f
davidgumberg:
Tested ACK 4e8ab5e00f
laanwj:
Code review ACK 4e8ab5e00f
Tree-SHA512: 680fb424f43b35730e03e0c7443c80445a2cf423d4f9161414ea22fea0b955f49197f8a96d1241896d981c6c13814d3eb7b5e4d8c9138813fb69e437ac4768ea
ff35a4b021 docs: Improve `keypoolrefill` RPC docs (w0xlt)
Pull request description:
Update `keypoolrefill` RPC docs to make it clear that descriptor wallets have 4 ScriptPubKeyManagers by default and each of them is updated in this command, as suggested https://github.com/bitcoin/bitcoin/issues/29924#issuecomment-2849321859
Closes https://github.com/bitcoin/bitcoin/issues/29924
ACKs for top commit:
achow101:
ACK ff35a4b021
brunoerg:
code review ACK ff35a4b021
Tree-SHA512: b6b9abe3fecebf9551b4ce9280794292c6ac44ccaeb2b9d60eeb4b2c177fe8372d0fe103f99c9cc0baeb2559ec019d1c495c233f24a600531153a38eeacb9670
de054df6dc contrib: Remove legacy wallet RPCs from bash completions (Ava Chow)
5dff04a1bb legacy spkm: Make IsMine() and CanProvide() private and migration only (Ava Chow)
c0f3f3264f wallet: Remove unused db functions (Ava Chow)
83af1a3cca wallet: Delete LegacySPKM (Ava Chow)
8ede6dea0c wallet, rpc: Remove legacy wallet only RPCs (Ava Chow)
4de3cec28d test: rpcs disabled for descriptor wallets will be removed (Ava Chow)
84f671b01d test: Run multisig script limit test (Ava Chow)
810476f31e test: Remove unused options and variables, correct comments (Ava Chow)
04a7a7a28c build, wallet, doc: Remove BDB (Ava Chow)
Pull request description:
The final step of #20160.
A bare minimum of legacy wallet code is kept in order to perform wallet migration. Migration of legacy wallets uses the independent BDB parser and a minimal `LegacyDataSPKM` that allows the legacy data to be loaded so that the migration can be completed.
BDB has been removed as a dependency and documentation have been updated to reflect that.
ACKs for top commit:
Sjors:
re-ACK de054df6dc
maflcko:
re-ACK de054df6dc🔗
w0xlt:
reACK de054df6dc
rkrux:
Concept ACK de054df6dc
Tree-SHA512: 16a6c265bc1ada5e7a5ef9b95f0ff65015672ca46d9a43b7e10d60e9e085052e9bbfe01ac3e494cc606afb652a1b476b10e434d13e9877b67d2cb0196a9bd190
85368aafa0 test: Run simple tests at various feerates (Murch)
d610951c15 test: Recreate BnB iteration exhaustion test (Murch)
2a1b2754f1 test: Remove redundant repeated test (Murch)
4781f5c8be test: Recreate simple BnB failure tests (Murch)
a94030ae98 test: Recreate BnB clone skipping test (Murch)
7db6f012c0 test: Move BnB feerate sensitivity tests (Murch)
2bafc46261 test: Recreate simple BnB success tests (Murch)
Pull request description:
This PR is splitting off some of the improvements made in #28985 and starts addressing the issues raised in #27754.
I aim to completely replace `coinselector_tests` with `coinselection_tests`. The goal is to generally use coins created per a nominal _effective value_ so we can get away from testing with `CoinSelectionParams` that are non-representative and effectuate counterintuitive behavior such as `feerate = 0` or `cost_of_change = 0`
ACKs for top commit:
achow101:
ACK 85368aafa0
monlovesmango:
ACK 85368aafa0
w0xlt:
ACK 85368aafa0
Tree-SHA512: 1a984837b4efddc0d8abe11668898fb207fb539e784bf911d4038211274b82e0fe1f8fffe7e5a19e0e013ccb7dc40e3f62d853a2a729980d0d935e66f12b9156
Add missing error check for fcntl(fd, F_GETFD, 0) in set_clo_on_exec.
Raise OSError on failure to align with existing FD_SETFD behavior.
This improves robustness in subprocess setup and error visibility.
Github-Pull: arun11299/cpp-subprocess#117
Rebased-From: 9974ff69cdd5fc1a2722cb63f006df9308628b35
This commit makes sure:
1. WaitForSingleObject returns with expected
code before proceeding.
2. Process handle is properly closed.
Github-Pull: arun11299/cpp-subprocess#116
Rebased-From: 625a8775791e62736f20f3fa3e6cc4f1b24aa89a
* refactor: Guard `util::quote_argument()` with `#ifdef __USING_WINDOWS__`
The `util::quote_argument()` function is specific to Windows and is used
in code already guarded by `#ifdef __USING_WINDOWS__`.
* Do not escape double quotes for command line arguments on Windows
This change fixes the handling of double quotes and aligns the behavior
with Python's `Popen` class. For example:
```
>py -3
>>> import subprocess
>>> p = subprocess.Popen("cmd.exe /c dir \"C:\\Program Files\"", stdout=subprocess.PIPE, text=True)
>>> print(f"Captured stdout:\n{stdout}")
```
Currently, the same command line processed by the `quote_argument()`
function looks like `cmd.exe /c dir "\"C:\Program" "Files\""`, which is
broken.
With this change, it looks correct: `cmd.exe /c dir "C:\Program Files"`.
Github-Pull: arun11299/cpp-subprocess#113
Rebased-From: ed313971c04ac10dc006104aba07d016ffc6542a
This suppresses the following warning caused by clang-20.
```
error: definition of implicit copy constructor for 'Streams' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
```
Copy constructor or move constructor is called when std::vector re-allocates
memory. In this case, move constructor should be called, because copying
Streams instances breaks file-descriptor management.
Communication class is modified as well, since it's instance is a member of
Streams class.
Github-Pull: arun11299/cpp-subprocess#107
Rebased-From: 38d98d9d20be50c7187b98ac9bc9a6e66920f6ef
The commit a32c0f3df4b6bcd1d2e93f19e8f380bb890cd507 introduced code to
silence MSVC's "warning C4996: The POSIX name for this item is
deprecated."
However, it exhibits several issues:
1. The aliases may leak into code outside the `subprocess.hpp` header.
2. They are unnecessarily applied when using the MinGW-w64 toolchain.
3. The fix is incomplete: downstream projects still see C4996 warnings.
4. The implementation lacks documentation.
This change addresses all of the above shortcomings.
Github-Pull: arun11299/cpp-subprocess#112
Rebased-From: 778543b2f2ca7f5d1c4f0241b635bbb265d750dd
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
Currently, wait() returns 0 on windows regardless
of the actual return code of processes.
Github-Pull: arun11299/cpp-subprocess#109
Rebased-From: 04b015a8e52ead4d8bb5f0eb486419c77e418a17
When passing in a rvalue reference, compiler
considers it ambiguous between std::string and
std::string&&. Making one of them take a lvalue
reference makes compilers correctly pick the right
one depending on whether the passed in value binds
to a rvalue or lvalue reference.
Github-Pull: arun11299/cpp-subprocess#110
Rebased-From: 2d8a8eebb03e509840e2c3c755d1abf32d930f33
I encountered this issue while running my code with Valgrind today.
Below is part of the Valgrind error message:
```
==1578139== 472 bytes in 1 blocks are still reachable in loss record 1 of 1
==1578139== at 0x4848899: malloc (...)
==1578139== by 0x4B3AF62: fdopen@@GLIBC_2.2.5 (...)
==1578139== by 0x118B09: subprocess::Popen::execute_process() (...)
```
I noticed that a similar fix had been proposed by another contributor
previously. I did not mean to scoop their work, but merely hoping to fix
it sooner so other people don't get confused by it just as I did today.
Github-Pull: arun11299/cpp-subprocess#106
Rebased-From: 3afe581c1f22f106d59cf54b9b65251e6c554671
e976bd3045 validation: add randomness to periodic write interval (Andrew Toth)
2e2f410681 refactor: replace m_last_write with m_next_write (Andrew Toth)
b557fa7a17 refactor: rename fDoFullFlush to should_write (Andrew Toth)
d73bd9fbe4 validation: write chainstate to disk every hour (Andrew Toth)
0ad7d7abdb test: chainstate write test for periodic chainstate flush (Andrew Toth)
Pull request description:
Since #28233, periodically writing the chainstate to disk every 24 hours does not clear the dbcache. Since #28280, periodically writing the chainstate to disk is proportional only to the amount of dirty entries in the cache. Due to these changes, it is no longer beneficial to only write the chainstate to disk every 24 hours. The periodic flush interval was necessary because every write of the chainstate would clear the dbcache. Now, we can get rid of the periodic flush interval and simply write the chainstate along with blocks and block index at least every hour.
Three benefits of doing this:
1. For IBD or reindex-chainstate with a combination of large dbcache setting, slow CPU, slow internet speed/unreliable peers, it could be up to 24 hours until the chainstate is persisted to disk. A power outage or crash could potentially lose up to 24 hours of progress. If there is a very large amount of dirty cache entries, writing to disk when a flush finally does occur will take a very long time. Crashing during this window of writing can cause https://github.com/bitcoin/bitcoin/issues/11600. By syncing every hour in unison with the block index we avoid this problem. Only a maximum of one hour of progress can be lost, and the window for crashing during writing is much smaller. For IBD with lower dbcache settings, faster CPU, or better internet speed/reliable peers, chainstate writes are already triggered more often than every hour so this change will have no effect on IBD.
2. Based on discussion in #28280, writing only once every 24 hours during long running operation of a node causes IO spikes. Writing smaller chainstate changes every hour like we do with blocks and block index will reduce IO spikes.
3. Faster shutdown speeds. All dirty chainstate entries must be persisted to disk on shutdown. If we have a lot of dirty entries, such as when close to 24 hours or if we sync with a large dbcache, it can take a long time to shutdown. By keeping the chainstate clean we avoid this problem.
Inspired by [this comment](https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2121088705).
Resolves https://github.com/bitcoin/bitcoin/issues/11600
ACKs for top commit:
achow101:
ACK e976bd3045
davidgumberg:
utACK e976bd3045
sipa:
utACK e976bd3045
l0rinc:
ACK e976bd3045
Tree-SHA512: 5bccd8f1dea47f9820a3fd32fe3bb6841c0167b3d6870cc8f3f7e2368f124af1a914bca6acb06889cd7183638a8dbdbace54d3237c3683f2b567eb7355e015ee
We do not need to repeat the same test multiple times because BnB is
deterministic and will therefore always have the same outcome.
Additionally, this test was redundant because it repeats the "Smallest
combination too big" test.
Originally these tests verified that at a SelectCoins level that a
solution with fewer inputs gets preferred at high feerates, and a
solution with more inputs gets preferred at low feerates. This outcome
relies on the behavior of BnB, so we move these tests under the umbrella
of BnB tests.
Originally these tests relied on SFFO to work.
Recreates the tests in a new test suite coinselection_tests.cpp that is
based on UTXOs being created per their effective values rather than
nominal values and uses transactions with non-zero feerates.
