40e5f26a3ff77e50df808f6f850c617aec2df203 mapport: remove dead code in DispatchMapPort (Antoine Poinsot)
38fdf7c1fb1946820236c319ad44c7bcbf0c6a98 mapport: drop outdated comments (Antoine Poinsot)
b7b24352906f1dba64826e7a093069b5bfc504dc doc: add release note for #31130 (Antoine Poinsot)
1b6dec98da3025c19951daf209347cecf1f0c6ab depends: drop miniupnpc (Antoine Poinsot)
953533d0214819a05d36672d295821ef06ced8d6 doc: remove mentions of UPnP (Antoine Poinsot)
94ad614482f4f1f9d207509a209badbc2fb5700d ci: remove UPnP options (Antoine Poinsot)
a9598e5eaab861fd6e6ce279f1282a83eec407d6 build: drop miniupnpc dependency (Antoine Poinsot)
a5fcfb7385c10d83a294cb2bb2248d06b2ab931e interfaces: remove now unused 'use_upnp' arg from 'mapPort' (Antoine Poinsot)
038bbe7b20074cc2201585dcc631e81b9e1e306c daemon: remove UPnP support (Antoine Poinsot)
844770b05ebc34789dc46d70cd6398089539c915 qt: remove UPnP settings (Antoine Poinsot)
Pull request description:
This PR removes UPnP IGD support and drops our [miniupnp](https://github.com/miniupnp/miniupnp) dependency.
Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed [here](https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=miniupnp)), some of which directly affected our software ([RCE in 2015](https://bitcoincore.org/en/2024/07/03/disclose_upnp_rce/), [OOM in 2020](https://bitcoincore.org/en/2024/07/31/disclose-upnp-oom/)).
The main purpose of this functionality is to have more (non-data-center) reachable nodes on the network. For a non-technical user running Bitcoin Core at home, the software would automatically open a port on their router to receive incoming connections. This way, users not able to manually open a port on their router would still provide the network with more resources and enhance its diversity.
However, due to past vulnerabilities (and a worry about unknown future ones) in miniupnpc this feature was disabled by default in https://github.com/bitcoin/bitcoin/pull/6795. Having it disabled by default kills (most of?) the purpose of having this functionality in the first place: someone technical enough to understand the `-upnp` startup option or the "enable UPnP" setting is most likely able to open a port on his box in the first place.
In addition, laanwj implemented PCP with a NAT-PMP fallback directly in Bitcoin Core in https://github.com/bitcoin/bitcoin/pull/30043. If we ever want to re-enable automatic NAT traversal by default in Bitcoin Core, this is the best option (and in my opinion the only sane one). The NAT-PMP fallback makes it so compatibility shouldn't be (much of) an issue.
On balance, i believe that keeping this functionality and this barely maintained C dependency has higher costs than benefits. Therefore i propose that we get rid of it.
ACKs for top commit:
jarolrod:
ACK 40e5f26a3f
1440000bytes:
Code Review ACK 40e5f26a3f
laanwj:
Code review ACK 40e5f26a3ff77e50df808f6f850c617aec2df203
i-am-yuvi:
Tested ACK 40e5f26a3ff77e50df808f6f850c617aec2df203
Tree-SHA512: 9ea48662775510f5ec6de7af65790f7c8d211603398e9d8c634a86387be81b28081419a95b4d6680d3d7fe6a9f16cec99f16516548201dc7e49781909899a657
4feaa28728442b0fd29a677d2b170a05fdf967c0 refactor: Rely on returned value of GetCoin instead of parameter (Lőrinc)
46dfbf169b49466de06dd16f7c23c6668419ef01 refactor: Return optional of Coin in GetCoin (Lőrinc)
e31bfb26c21f8b72f8c317e016d375862e27502e refactor: Remove unrealistic simulation state (Lőrinc)
Pull request description:
While reviewing [the removal of the unreachable combinations from the Coin cache logic](https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721727681), we've noticed that the related tests often [reflect impossible states](https://github.com/bitcoin/bitcoin/pull/30673/files#r1740154464).
Browsing the Coin cache refactoring history revealed that migrating `bool GetCoin` to `optional<Coin> GetCoin` was [already proposed a few times before](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).
This refactor makes certain invalid states impossible, reducing the possibility of errors and making the code easier to understand. This will let us remove test code that exercises the impossible states as well.
The PR is done in multiple small steps, first swapping the new `optional` return value, slowly strangling out the usages of the return parameter, followed by the removal of the parameter.
Most of the invalid test states were still kept, except for https://github.com/bitcoin/bitcoin/pull/30673/files#r1748087322, where the new design prohibits invalid usage and https://github.com/bitcoin/bitcoin/pull/30673/files#r1749350258 was just marked with a TODO, will be removed in a follow-up PR.
ACKs for top commit:
andrewtoth:
re-ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
achow101:
ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
laanwj:
Code review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
theStack:
Code-review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
Tree-SHA512: 818d60b2e97f58c489a61120fe761fb67a08dffbefe7a3fce712d362fc9eb8c2cced23074f1bec55fe71c616a3561b5a8737919ad6ffb2635467ec4711683df7
31cc5006c3de4dd6a1f7a238684163956604df45 init: Return fatal failure on snapshot validation failure (Martin Zumsande)
8f1246e833804789ee5d8b59026b49142df5c455 init: Improve chainstate init db error messages (TheCharlatan)
cd093049dda878e8424fdc1ef828b5f644bd91d4 init: Remove incorrect comment about shutdown condition (MarcoFalke)
635e9f85d76c28647120172d9524982ebe36cf3c init: Remove misleading log line when user chooses not to retry (TheCharlatan)
720ce880a355cf59a4f042a504750eb4e3ee68d3 init: Improve comment describing chainstate load retry behaviour (Martin Zumsande)
baea842ff184f98d2f07568f0a77e48a34d3cde3 init: Remove unneeded argument for mempool_opts checks (stickies-v)
Pull request description:
These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.
The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.
ACKs for top commit:
achow101:
ACK 31cc5006c3de4dd6a1f7a238684163956604df45
mzumsande:
Code Review / lightly tested ACK 31cc5006c3de4dd6a1f7a238684163956604df45
BrandonOdiwuor:
Code Review ACK 31cc5006c3de4dd6a1f7a238684163956604df45.
stickies-v:
ACK 31cc5006c3de4dd6a1f7a238684163956604df45
Tree-SHA512: 59fba4845ee45a3d91bf55807ae6b1c81458463b96bf664c8b1badfac503f6b01efd52a915fc399294e68a3f69985362a5a10a3844fa23f7707145ebe9ad349b
525e9dcba0b8c6744bcd3725864f39786afc8ed5 Add submitSolution to BlockTemplate interface (Sjors Provoost)
47b4875ef050c4a41bd04398021af8d605415cab Add getCoinbaseMerklePath() to Mining interface (Sjors Provoost)
63d6ad7c89cd682f6e779968c4861ea085058356 Move BlockMerkleBranch back to merkle.{h,cpp} (Sjors Provoost)
Pull request description:
The new `BlockTemplate` interface introduced in #30440 allows for a more efficient way for a miner to submit the block solution. Instead of having the send the full block, it only needs to provide the nonce, timestamp, version fields and coinbase transaction.
This PR introduces `submitSolution()` for that. It's currently unused.
#29432 and https://github.com/Sjors/bitcoin/pull/48 use it to process the Stratum v2 message [SubmitSolution](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#77-submitsolution-client---server). The method should be sufficiently generic to work with alternative mining protocols (none exist that I'm aware off).
This PR also introduces `getCoinbaseMerklePath()`, which is needed in Stratum v2 to construct the `merkle_path` field of the `NewTemplate` message (see [spec](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client)). The coinbase merkle path is also used in Stratum "v1", see e.g. https://bitcoin.stackexchange.com/questions/109820/questions-on-merkle-root-hashing-for-stratum-pools
This last function uses `BlockMerkleBranch` which was moved to the test code in #13191. The reason back then for moving it was that it was no longer used. This PR moves it back.
This PR does not change behaviour since both methods are unused.
ACKs for top commit:
achow101:
ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
itornaza:
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
tdb3:
Code review and light test ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
ryanofsky:
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5. Left minor suggestions but none are important, and looks like this could be merged as-is
Tree-SHA512: 2a6a8f5d409ff4926643193cb67702240c7c687615414371e53383d2c13c485807f65e21e8ed98515b5456eca3d9fca13cec04675814a4081467d88b849c5653
Instead of having a single NodeContext::shutdown member that is used both to
request shutdowns and check if they have been requested, use separate members
for each. Benefits of this change:
1. Should make code a little clearer and easier to search because it is easier
to see which parts of code are triggering shutdowns and which parts are just
checking to see if they were triggered.
2. Makes it possible for init.cpp to specify additional code to run when a
shutdown is requested, like signalling the m_tip_block_cv condition variable.
Motivation for this change was to remove hacky NodeContext argument and
m_tip_block_cv access from the StopRPC function, so StopRPC can just be
concerned with RPC functionality, not other node functionality.
This is not strictly required, but all places using m_tip_block_cv
(except shutdown) already take the lock. The annotation makes it easier
to catch potential deadlocks before review.
Adding the missing lock to the shutdown sequence is a bugfix.
An alternative would be to take the lock and release it before
notifying, see
https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716
5c7cacf649a6b474b876a7d219c7dc683a25e33d ci: Remove natpmp build option and libnatpmp dependency (laanwj)
7e7ec984da50f45491b994aaab180e7735ad1d8f doc: Remove mention of natpmp build options (laanwj)
061c3e32a26c6c04bf734d62627403758d7e51d9 depends: Drop natpmp and associated option from depends (laanwj)
20a18bf6aa38e87f72e2645482d00d0c77a344f5 build: Drop libnatpmp from build system (laanwj)
7b04709862f48e9020c7bef79cb31dd794cf91d0 qt: Changes for built-in PCP+NAT-PMP (laanwj)
52f8ef66c61b82457a161f3b90cc87f57d1dda80 net: Replace libnatpmp with built-in NATPMP+PCP implementation in mapport (laanwj)
97c97177cdb2f596aa7d4a65c4bde87de50a96f2 net: Add PCP and NATPMP implementation (laanwj)
d72df63d16941576b3523cfeaa49985cf3cd4d42 net: Use GetLocalAddresses in Discover (laanwj)
e02030432b77abbf27bb4f67d879d3ad6d6366e6 net: Add netif utility (laanwj)
754e4254388ec8ac1be6cf807bf300cd43fd3da5 crypto: Add missing WriteBE16 function (laanwj)
Pull request description:
Continues #30005. Closes #17012..
This PR adds PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse.
For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface.
To test:
```bash
bitcoind -regtest -natpmp=1 -debug=net
```
(most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs)
## TODO
- [x] Default gateway discovery on Linux / FreeBSD
- [x] Default gateway discovery on Windows
- [x] Default gateway discovery on MacOS
- [x] Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support
## Things to consider for follow-up PRs
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658764974 avoid unreachable nets (not given to -onlynet=)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236 could announce an addr:port where we do not listen (no -bind)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1684368824 could announce the wrong port because it uses GetListenPort()
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679709347 if we requested one port but another was assigned, then which one to use in the renewal?
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020 Use `GetAdapterAddresses` to discover local addresses for Windows
ACKs for top commit:
Sjors:
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d
achow101:
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d
vasild:
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d
Tree-SHA512: e35b69e56d5f5449a3d48a799f9b7b65107c65eeb3e245c2c1e9d42221e469ca5ead90afae423160601cd664dd553a51c859e04f4492f335b064aae3bf23e3bc
7942951e3fcc58f7db0059546d03be9ea243f1db Remove unused g_best_block (Ryan Ofsky)
e3a560ca68d79b056a105a65ed0c174a9631aba9 rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a09c2af336fce853d9ffb790d01429eec6 Remove unused CRPCSignals (Sjors Provoost)
dca923150e3ac10a57c23a7e29e76516d32ec10d Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1121903847cdd3f6c5b4796e4d5471b2df rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855b3af99fe6b62279c5c2d08888a5437c4a rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072925a6d558b3c6b075becbed44727c5989 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9fb51c808a9edd91b05bd3134fc18de0fb6 rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27cf05c709674117e308e441a8d1efddcd0a Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf16081d6f624c4dc21df75b0474e049d2b node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215f23644f901c46fd4977b7d4b08fae5104 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74bbbb900abfb3d8e946eea18ad7b1513ad refactor: rename BlockKey to BlockRef (Sjors Provoost)
Pull request description:
This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.
`waitTipChanged()` uses a new kernel notification `notifications().m_tip_block_mutex`, which this PR also introduces (a previous version used `g_best_block`).
In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.
There's a commit to add (direct) tests for the methods that are about to be refactored:
- `waitfornewblock` was already implicitly tested by `feature_shutdown.py`.
- `waitforblockheight` by `feature_coinstatsindex.py` and `example_test.py`
This PR renames `getTipHash()` to `getTip()` and returns a `BlockRef` (renamed from `BlockKey`) so that callers can use either the height or hash.
The later commits make trivial improvements to the `waitfor*` RPC calls (not needed for this PR).
The `waitTipChanged()` method could probably also be used for the longpoll functionality in `getblocktemplate`, but I'm a bit reluctant to touch that.
`RPCServer::OnStarted` no longer does anything and `RPCServer::OnStopped` merely prints a log statement. They were added in #5711 as a refactor. This PR drops them entirely.
Finally `g_best_block` is also dropped.
ACKs for top commit:
achow101:
ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
ryanofsky:
Code review ACK 7942951e3fcc58f7db0059546d03be9ea243f1db. Just rebased since last review
TheCharlatan:
Re-ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
Tree-SHA512: a5559446b4000c95e07aad33284b7ee2e57aafd87e1ae778b3825d59689566d047a8047e47a10f76e6e341e7dc72fd265a65afbc0a9c011d17c4cafd55031837
84663291275248fd52da644b0c2566bbf9cc780b chain: simplify `deleteRwSettings` code and improve it's doc (ismaelsadeeq)
f8d91f49c7091102138fcb123850399e8fadfbc7 chain: dont check for null settings value in `overwriteRwSetting` (ismaelsadeeq)
df601993f2d7454f081316d6a8ddefbcefa49b3d chain: ensure `updateRwSetting` doesn't update to a null settings (ismaelsadeeq)
c8e2eeeffb494d99d95c1c45efeda5a98dce94cd chain: uniformly use `SettingsAction` enum in settings methods (ismaelsadeeq)
1e9e735670f029c975d3b7486a54e5bb67135df7 chain: move new settings safely in `overwriteRwSetting` (ismaelsadeeq)
1c409004c80bc5f2314e20cc922076e22931cf73 test: remove wallet context from `write_wallet_settings_concurrently` (ismaelsadeeq)
Pull request description:
This PR addresses the remaining review comments from #30697
1. Disallowed overwriting settings values with a `null` value.
2. Uniformly used the `SettingsAction` enum in all settings methods instead of a boolean parameter.
3. Updated `overwriteRwSetting` to receive the `common::SettingsValue` parameter by value, enabling it to be moved safely.
4. Removed wallet context from the `write_wallet_settings_concurrently` unit test, as it is not needed.
ACKs for top commit:
achow101:
ACK 84663291275248fd52da644b0c2566bbf9cc780b
ryanofsky:
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b. Looks good, thanks for taking suggestions and applying them to the right commits. Only changes since last review were documentation improvements and simplifying delete method.
furszy:
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b
Tree-SHA512: baf2f59ed5aac4a4bda0c84fb6554a466a40d1f7b52b61dc2ff293d83ae60e82b925b7003237b633fecb65eba3a4c108e69166046895d1295809fbe0de67b052
Also signal m_tip_block_cv when StopRPC is called, for
consistency with g_best_block_cv. This is handled in
StopRPC instead of OnRPCStopped() because the latter
is deleted in a later commit.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
a240e150e837b5a95ed19765a2e8b7c5b6013f35 streams: remove AutoFile::Get() entirely (Pieter Wuille)
e624a9bef16b6335fd119c10698352b59bf2930a streams: cache file position within AutoFile (Pieter Wuille)
Pull request description:
Fixes#30833.
Instead of relying on frequent `ftell` calls (which appear to cause a significant slowdown on some systems) in XOR-enabled `AutoFile`s, cache the file position within `AutoFile` itself.
ACKs for top commit:
achow101:
ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
davidgumberg:
untested reACK a240e150e8
theStack:
Code-review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
Tree-SHA512: fd3681edc018afaf955dc7a41a0c953ca80d46c1129e3c5b306c87c95aae93b2fe7b900794eb8b6f10491f9211645e7939918a28838295e6873eb226fca7006f
bb3b980dfd96d9a89e6b04aef2859ce0555c6807 validation: drop maximum -dbcache (Sjors Provoost)
Pull request description:
Due to recent UTXO set growth, the current maximum value for `-dbcache` of 16GB is ~just months away from being~ insufficient (for those who wish to complete IBD with the UTXO set held in RAM).
This drops the limit. It also adds a warning that it's up to users to check that they have enough RAM.
Fixes#28249.
---
A previous version of this PR increased the maximum to 64GB. It also made startup abort if the value provided is too high, rather than quietly round it down. But this didn't get much support.
ACKs for top commit:
achow101:
ACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807
tdb3:
ACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807
BenWestgate:
crACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807.
Tree-SHA512: 8515fff468c2387a0b04bd9523ab1df46d6325738588b7550fabddbb8624817a583d95b95ea246407f9f0ff3e43e760cf7334621bec6af79710176328528a3ef
a93c171faa7b4426823466e972c8f24260918bbf Drop unneeded nullptr check from CreateNewBlock() (Sjors Provoost)
dd87b6dff352c408e90e3d612d773ff9562d2382 Have createNewBlock return BlockTemplate interface (Sjors Provoost)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a [NewTemplate](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client) message message (which doesn't include transactions) as quickly as possible. It does not include the serialized block transactions.
ACKs for top commit:
achow101:
ACK a93c171faa7b4426823466e972c8f24260918bbf
ryanofsky:
Code review ACK a93c171faa7b4426823466e972c8f24260918bbf. Since last review, just rebased with no changes or conflicts
itornaza:
Code review ACK a93c171faa7b4426823466e972c8f24260918bbf
TheCharlatan:
Re-ACK a93c171faa7b4426823466e972c8f24260918bbf
Tree-SHA512: 17cb61eb5548e9d4a69e34dd732f68a06cde2ad3d82c8339efee704c7860d5de144d93b23d6ecd6ee4ec205844e5560ad0f6d3917822fa75bb8e640c5f51af9a
The thread handle is never used by the ChainstateManager, so move it out
and into the node context. Users of the kernel library now no longer
have to manually join the thread when destructing the ChainstateManager.
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a NewTemplate message (which doesn't include transactions) as quickly as possible.
b31a0cd0378184b2b9eb8f4bd3120cbd32c62005 log: expand BCLog::LogFlags (categories) to 64 bits (Larry Ruane)
Pull request description:
Increase the maximum number of logging categories from 32 to 64.
We're currently using 29 of the 32 available logging categories (there are only 3 remaining). It would be good to increase the limit soon; the fourth PR to be merged that adds a new logging category will be blocked until something like this is done.
This PR also adds a `TEST` category that uses the new range (`1ULL << 63`) in case there's a hidden assumption somewhere that the `BCLog::LogFlags` type is 32 bits. (Also added a test for this test category.) It also provides an example showing that the expression must be `1ULL << <shift>` for shift value 31 and beyond.
ACKs for top commit:
achow101:
ACK b31a0cd0378184b2b9eb8f4bd3120cbd32c62005
vasild:
ACK b31a0cd0378184b2b9eb8f4bd3120cbd32c62005
ryanofsky:
Code review ACK b31a0cd0378184b2b9eb8f4bd3120cbd32c62005, just dropping mask_bit constant since last review. I still think
theStack:
Code-review ACK b31a0cd0378184b2b9eb8f4bd3120cbd32c62005
Tree-SHA512: de422dbeb479848d370aed42d415f42461457ab0eda62b245dc7ff9f0e111626e7d4c0d62ff13082ec664d05fbb0db04c71eb4b6f22eb8f19198826a67c4035e
a2955f09792b6232f3a45aa44a498b466279a8b7 validation: Use span for ImportBlocks paths (TheCharlatan)
20515ea3f5bd426f6e3746cf5cddd2324dacae31 validation: Use span for CalculateClaimedHeadersWork (TheCharlatan)
52575e96e72a0402c448f86728b2e84836b1e817 validation: Use span for ProcessNewBlockHeaders (TheCharlatan)
Pull request description:
Makes it friendlier for potential future users of the kernel library if they do not store the headers in a std::vector, but can guarantee contiguous memory.
Take this opportunity to also change the argument of ImportBlocks previously taking a `std::vector` to a `std::span`.
ACKs for top commit:
stickies-v:
re-ACK a2955f09792b6232f3a45aa44a498b466279a8b7 - no changes except further walking the ~file~ path of modernizing variable names.
maflcko:
ACK a2955f09792b6232f3a45aa44a498b466279a8b7 🕑
achow101:
ACK a2955f09792b6232f3a45aa44a498b466279a8b7
danielabrozzoni:
ACK a2955f09792b6232f3a45aa44a498b466279a8b7
Tree-SHA512: 8b07f4ad26e270b65600d1968cd78847b85caca5bfbb83fd9860389f26656b1d9a40b85e0990339f50403d18cedcd2456990054f3b8b0bedce943e50222d2709
Makes it friendlier for potential future users of the kernel library if
they do not store the headers in a std::vector, but can guarantee
contiguous memory.
18d65d27726bf9fc7629b8e794047a10c9cf6156 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a329c3bf38e47a07434e2a3c0031f808d0 node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb544b538dba9823bcd5754d074272bfc04 util: remove unused IsHexNumber (stickies-v)
8a44d7d3c1e5d5af6779c3e4befe514c9dafb8ff node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87737e77ee85812cc328c4ddfaea7147533 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbfcbe3f94770bdf73dedd8bda0193a44627 test: unittest chainstatemanager_args (stickies-v)
Pull request description:
Since fad2991ba073de0bd1f12e42bf0fbaca4a265508, `uint256S` has been [deprecated](fad2991ba0 (diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138)) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. _(see also #30532)_
This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change.
The main behaviour change introduced in this PR is:
- `-minimumchainwork` will raise an error when input is longer than 64 hex digits
- `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits
- test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits
After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.
ACKs for top commit:
hodlinator:
re-ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
l0rinc:
ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
achow101:
ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
ryanofsky:
Code review ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.
Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86
1b41d45d462d856a9d0b44ae0039bbb2cd78407c wallet: bugfix: ensure atomicity in settings updates (ismaelsadeeq)
Pull request description:
This PR fixes#30620.
As outlined in the issue, creating two wallets with `load_on_startup=true` simultaneously results in only one wallet being added to the startup file.
The current issue arises because the wallet settings update process involves:
1. Obtaining the settings value while acquiring the settings lock.
2. Modifying the settings value.
3. Overwriting the settings value while acquiring the settings lock again.
This sequence is not thread-safe. Different threads could modify the same base value simultaneously, overwriting data from other workers without realizing it.
The PR attempts to fix this by modifying the chain interface's `updateRwSetting` method to accept a function that will be called with the settings reference. This function will either update or delete the setting and return an enum indicating whether the settings need to be overwritten in this or not.
Additionally, this PR introduces two new methods to the chain interface:
- `overwriteRwSetting`: This method replaces the setting with a new value.
Used in `VerifyWallets`
- `deleteRwSettings`: This method completely erases a specified setting.
This method is currently used only in `overwriteRwSetting`.
These changes ensure that updates are race-free across all clients.
ACKs for top commit:
achow101:
ACK 1b41d45d462d856a9d0b44ae0039bbb2cd78407c
furszy:
self-code-ACK 1b41d45d46
Tree-SHA512: 50cda612b782aeb5e03e2cf63cc44779a013de1c535b883b57af4de22f24b0de80b4edecbcda235413baec0a12bdf0e5750fb6731c9e67d32e742d8c63f08c13
- Settings updates were not thread-safe, as they were executed in
three separate steps:
1) Obtain settings value while acquiring the settings lock.
2) Modify settings value.
3) Overwrite settings value while acquiring the settings lock.
This approach allowed concurrent threads to modify the same base value
simultaneously, leading to data loss. When this occurred, the final
settings state would only reflect the changes from the last thread
that completed the operation, overwriting updates from other threads.
Fix this by making the settings update operation atomic.
- Add test coverage for this behavior.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Removes dependency on unsafe and deprecated uint256S.
This makes parsing more strict, by returning an error
when the input contains non-hex characters, or when it
contains more than 64 hex digits.
Also make feature_assumevalid.py more robust by using CBlock.hash
which is guaranteed to be 64 characters long, as opposed to the
variable-length hex(CBlock.sha256)
Removes dependency on unsafe and deprecated uint256S.
This makes parsing more strict, by returning an error
when the input contains more than 64 hex digits.
00618e8745192d209c23e3ae873c077e58168957 assumeutxo: Drop block height from metadata (Fabian Jahr)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/30514 which has more context and shows how the issue can be reproduced. Since the value in question is removed, there is no test to add to reproduce anything.
This is an alternative approach to #30516 with much of the [code being suggested there](https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1689146902).
ACKs for top commit:
maflcko:
re-ACK 00618e8745192d209c23e3ae873c077e58168957 🎌
achow101:
ACK 00618e8745192d209c23e3ae873c077e58168957
theStack:
Code-review ACK 00618e8745192d209c23e3ae873c077e58168957
ismaelsadeeq:
Re-ACK 00618e8745192d209c23e3ae873c077e58168957
mzumsande:
ACK 00618e8745192d209c23e3ae873c077e58168957
Tree-SHA512: db9575247bae838ad7742a27a216faaf55bb11e022f9afdd05752bb09bbf9614717d0ad64304ff5722a16bf41d8dea888af544e4ae26dcaa528c1add0269a4a8
