Commit graph

42934 commits

Author SHA1 Message Date
TheCharlatan
1f7fc73825
rpc: Remove submitblock duplicate pre-check
The duplicate checks are repeated early in the contextual checks of
ProcessNewBlock. If duplicate blocks are detected much of their
validation is skipped. Depending on the constitution of the block,
validating the merkle root of the block is part of the more intensive
workload when validating a block. This could be an argument for moving
the pre-checks into block processing. In net_processing this would have
a smaller effect however, since the block mutation check, which also
validates the merkle root, is done before.

A side effect of this change is that a duplicate block is persisted
again on disk even when pruning is activated. This is similar to the
behaviour with getblockfrompeer. Add a release note for this change in
behaviour.

Testing spamming a node with valid, but duplicate unrequested blocks
seems to exhaust a CPU thread, but does not seem to significantly impact
keeping up with the tip. The benefits of adding these checks to
net_processing are questionable, especially since there are other ways
to trigger the more CPU-intensive checks without submitting a duplicate
block. Since these DOS concerns apply even less to the RPC interface,
which does not have banning mechanics built in, remove them too.

---

With the introduction of a mining ipc interface and the potential future
introduction of a kernel library API it becomes increasingly important
to offer common behaviour between them. An example of this is
ProcessNewBlock, which is used by ipc, rpc, net_processing and
(potentially) the kernel library. Having divergent behaviour on
suggested pre-checks and checks for these functions is confusing to both
developers and users and is a maintenance burden.

The rpc interface for ProcessNewBlock (submitblock) currently pre-checks
if the block has a coinbase transaction and whether it has been
processed before. While the current example binary for how to use the
kernel library, bitcoin-chainstate, imitates these checks, the other
interfaces do not.
2024-11-21 22:18:33 +01:00
TheCharlatan
e62a8abd7d
rpc: Remove submitblock invalid-duplicate precheck
ProcessNewBlock fails if an invalid duplicate block is passed in through
its call to AcceptBlock and AcceptBlockHeader. The failure in
AcceptBlockHeader makes AcceptBlock return early. This makes the
pre-check in submitblock redundant.

---

With the introduction of a mining ipc interface and the potential future
introduction of a kernel library API it becomes increasingly important
to offer common behaviour between them. An example of this is
ProcessNewBlock, which is used by ipc, rpc, net_processing and
(potentially) the kernel library. Having divergent behaviour on
suggested pre-checks and checks for these functions is confusing to both
developers and users and is a maintenance burden.

The rpc interface for ProcessNewBlock (submitblock) currently pre-checks
if the block has a coinbase transaction and whether it has been
processed before. While the current example binary for how to use the
kernel library, bitcoin-chainstate, imitates these checks, the other
interfaces do not.
2024-11-21 22:18:31 +01:00
TheCharlatan
36dbebafb9
rpc: Remove submitblock coinbase pre-check
The coinbase check is repeated again early during ProcessNewBlock.
Pre-checking it may also shadow more fundamental problems with a block.
In most cases the block header is checked first, before validating the
transactions. Checking the coinbase first therefore masks potential
issues with the header. Fix this by removing the pre-check.

The pre-check was likely introduced on top of
ada0caa165 to fix UB in
GetWitnessCommitmentIndex in case a block's transactions are empty. This
code path could only be reached because of the call to
UpdateUncommittedBlockStructures in submitblock, but cannot be reached
through net_processing.

Add some functional test cases to cover the previous conditions that
lead to a "Block does not start with a coinbase" json rpc error being
returned.

---

With the introduction of a mining ipc interface and the potential future
introduction of a kernel library API it becomes increasingly important
to offer common behaviour between them. An example of this is
ProcessNewBlock, which is used by ipc, rpc, net_processing and
(potentially) the kernel library. Having divergent behaviour on
suggested pre-checks and checks for these functions is confusing to both
developers and users and is a maintenance burden.

The rpc interface for ProcessNewBlock (submitblock) currently pre-checks
if the block has a coinbase transaction and whether it has been
processed before. While the current example binary for how to use the
kernel library, bitcoin-chainstate, imitates these checks, the other
interfaces do not.
2024-11-21 22:16:43 +01:00
merge-script
da10e0bab4
Merge bitcoin/bitcoin#30942: test: Remove dead code from interface_zmq test
Some checks are pending
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
c4dc81f9c6 test: Remove dead code from interface_zmq (Fabian Jahr)

Pull request description:

  The loop removed here appears to be effectively dead code: In case `get_raw_seq` is behind `zmq_mem_seq` the loop runs and tries to get a more recent (higher) number for `get_raw_seq`. However, the exact number of `get_raw_seq` is asserted in the line above: `assert_equal(get_raw_seq, 6)`. If the loop would actually achieve its purpose this assert would need to be racy. This does not seem to be the case and 6 appears to be the final number. `zmq_mem_seq` however does take some time to catch up (if it were continue to be updated). But this is not handled by the loop and does not seem to be relevant at this point in the test. The backlog is consumed a bit later in another loop that handles this correctly already.

ACKs for top commit:
  l0rinc:
    ACK c4dc81f9c6
  tdb3:
    CR re ACK c4dc81f9c6

Tree-SHA512: 663a1711ba1ce04a3d2e2916e0df7a7bb51069e28bc2644b816a483628c95b5e6c29fc6eacc31a5f72b7d9af11096f3c437ea1dc57eaa1ee9ddce43cc20bacd3
2024-10-28 16:32:21 +00:00
merge-script
e96ffa98b0
Merge bitcoin/bitcoin#31142: test: fix intermittent failure in p2p_seednode.py, don't connect to random IPs
6c9fe7b73e test: Prevent connection attempts to random IPs in p2p_seednodes.py (Martin Zumsande)
bb97b1ffa9 test: fix intermittent timeout in p2p_seednodes.py (Martin Zumsande)

Pull request description:

  Fixes #31103

  On some CI runs, the seed node timer in `ThreadOpenConnection` was only started *after* the mocktime was set.
  Fix this by waiting for the first connection attempt, which happens after the timer was started.

  Also I noticed that the "unreachable" connections are not in fact unreachable, so that the functional test could attempt connections
  to random IPs on the internet. This was already noted in https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1701616675 but the suggested fix never made it in, so I added it to this PR.

ACKs for top commit:
  sr-gi:
    tACK [6c9fe7b](6c9fe7b73e)
  laanwj:
    Code review ACK 6c9fe7b73e
  tdb3:
    cr and light test ACK 6c9fe7b73e

Tree-SHA512: 021b6d5325eab85d79708b4b137f61723a36f2b8a1faf681463bad2ea5283ea528b5ff1701467a86b035d3a6972750a61ace5020e58b7aa61ecaad97664488c8
2024-10-28 15:50:36 +00:00
merge-script
54c4b09f08
Merge bitcoin/bitcoin#31042: build: Rename PACKAGE_* variables to CLIENT_*
70713303b6 scripted-diff: Rename `PACKAGE_*` variables to `CLIENT_*` (Hennadii Stepanov)
332655cb52 build: Rename `PACKAGE_*` variables to `CLIENT_*` (Hennadii Stepanov)
e6e29e3c94 scripted-diff: Clarify "user agent" variable name (Hennadii Stepanov)

Pull request description:

  The use of `PACKAGE_NAME` for the project's variable name is problematic, as this name is commonly used in CMake's [interface variables](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-version-selection). If third-party CMake code handles with scopes improperly, our `PACKAGE_NAME` variable could end up with an unexpected value.

  This PR avoids such conflicts by renaming all `PACKAGE_*` variables to `CLIENT_*`.

  The code in the master branch works correctly only incidentally. It is definitely broken in https://github.com/bitcoin/bitcoin/pull/30997.

ACKs for top commit:
  TheCharlatan:
    Re-ACK 70713303b6

Tree-SHA512: f0992fb9a1ac4a41125b223f97bcaae50f521c813f334e606693dd0113a4732f12451bbcfb158df8bed44f34b37dadac478b2f5661e8b2588b401f43ae4bc1a4
2024-10-28 15:45:09 +00:00
Hennadii Stepanov
70713303b6
scripted-diff: Rename PACKAGE_* variables to CLIENT_*
This change ensures consistent use of the `CLIENT_` namespace everywhere
in the repository.

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./cmake ./src :\(exclude\)./src/secp256k1 ./test ) ; }

ren PACKAGE_NAME      CLIENT_NAME
ren PACKAGE_VERSION   CLIENT_VERSION_STRING
ren PACKAGE_URL       CLIENT_URL
ren PACKAGE_BUGREPORT CLIENT_BUGREPORT

-END VERIFY SCRIPT-
2024-10-28 12:36:19 +00:00
Hennadii Stepanov
332655cb52
build: Rename PACKAGE_* variables to CLIENT_*
The use of `PACKAGE_NAME` for the project's variable name is
problematic, as this name is commonly used in CMake's interface
variables. If third-party CMake code handles with scopes improperly,
our `PACKAGE_NAME` variable could end up with an unexpected value.

This change avoids such conflicts by renaming all `PACKAGE_*` variables
to `CLIENT_*`.
2024-10-28 12:35:55 +00:00
Hennadii Stepanov
e6e29e3c94
scripted-diff: Clarify "user agent" variable name
This change allows to the use of the `CLIENT_` namespace without
potential name clashes.

-BEGIN VERIFY SCRIPT-
sed -i "s/\<CLIENT_NAME\>/UA_NAME/g" $( git grep -l "CLIENT_NAME" ./src)
-END VERIFY SCRIPT-
2024-10-28 12:35:49 +00:00
merge-script
1c7ca6e64d
Merge bitcoin/bitcoin#31093: Introduce g_fuzzing global for fuzzing checks
9f243cd7fa Introduce `g_fuzzing` global for fuzzing checks (dergoegge)

Pull request description:

  This PR introduces a global `g_fuzzing` that indicates if we are fuzzing.

  If `g_fuzzing` is `true` then:

  * Assume checks are enabled
  * Special fuzzing paths are taken (e.g. pow check is reduced to one bit)

  Closes #30950 #31057

ACKs for top commit:
  maflcko:
    review ACK 9f243cd7fa 🗜
  brunoerg:
    crACK 9f243cd7fa
  marcofleon:
    Tested ACK 9f243cd7fa

Tree-SHA512: 56e4cad0555dec0c565ea5ecc529628ee4f37d20dc660c647fdc6948fbeed8291e6fe290de514bd4c2c7089654d9ce1add607dc9855462828b62be9ee45e4999
2024-10-28 11:05:50 +00:00
merge-script
6e21dedbf2
Merge bitcoin/bitcoin#31130: Drop miniupnp dependency
40e5f26a3f mapport: remove dead code in DispatchMapPort (Antoine Poinsot)
38fdf7c1fb mapport: drop outdated comments (Antoine Poinsot)
b7b2435290 doc: add release note for #31130 (Antoine Poinsot)
1b6dec98da depends: drop miniupnpc (Antoine Poinsot)
953533d021 doc: remove mentions of UPnP (Antoine Poinsot)
94ad614482 ci: remove UPnP options (Antoine Poinsot)
a9598e5eaa build: drop miniupnpc dependency (Antoine Poinsot)
a5fcfb7385 interfaces: remove now unused 'use_upnp' arg from 'mapPort' (Antoine Poinsot)
038bbe7b20 daemon: remove UPnP support (Antoine Poinsot)
844770b05e 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 40e5f26a3f
  i-am-yuvi:
    Tested ACK 40e5f26a3f

Tree-SHA512: 9ea48662775510f5ec6de7af65790f7c8d211603398e9d8c634a86387be81b28081419a95b4d6680d3d7fe6a9f16cec99f16516548201dc7e49781909899a657
2024-10-28 10:47:34 +00:00
glozow
2a52718d73
Merge bitcoin/bitcoin#31152: functional test: Additional package evaluation coverage
Some checks failed
CI / test each commit (push) Has been cancelled
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Has been cancelled
CI / Win64 native, VS 2022 (push) Has been cancelled
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Has been cancelled
f32c34d0c3 functional test: Additional package evaluation coverage (Greg Sanders)

Pull request description:

  Current test coverage doesn't ensure that mempool trimming doesn't appear prior to the entire package, and not just the subpackage, is finished being submitted.

  Add a scenario that covers this case, where package ancestors can make it in individually, but would be immadiately evicted if not for the package CPFP.

  in response to https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813272637 where if applied onto that PR's old commit, the test fails due to package failure.

ACKs for top commit:
  sdaftuar:
    re-ACK f32c34d0c3
  rkrux:
    tACK f32c34d0c3
  glozow:
    reACK f32c34d0c3

Tree-SHA512: 739fcc5e66878b3def9b25dc588d8cb5349aaaa0901b11475879a413a03f6ea0e87d19de5bc4fb44ddd0436fdc052cdc3ed564f7e2ad510269aab9732d5c24eb
2024-10-26 09:37:20 -04:00
Ava Chow
25dacae9c7
Merge bitcoin/bitcoin#31040: test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
5c299ecafe test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped (kevkevinpal)

Pull request description:

  After joining the bitcoin pr review club about https://github.com/bitcoin/bitcoin/pull/30793

  I learned about [`CVE-2012-3789`](https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L4693)

  So I was motivated to write a functional test that covers this part of the code,

  This test should add the max number of orphans to a nodes orphanage and then attempt to add another, then asserts that the number of orphans is still at the max amount

ACKs for top commit:
  achow101:
    ACK 5c299ecafe
  rkrux:
    ACK 5c299ecafe
  instagibbs:
    ACK 5c299ecafe
  tdb3:
    ACK 5c299ecafe

Tree-SHA512: 687bba337978e0945e94af71632998221e5565a5d83cf5a59ecf2ee52c7262d8ff907b94dceea3b80bed441dd19b24790b2904e88e1da14d30827c5469fcb4d3
2024-10-25 16:35:18 -04:00
Antoine Poinsot
40e5f26a3f
mapport: remove dead code in DispatchMapPort
Since there is now only two options in the MapPortProtoFlag enum, the
four possible combinations of current and enabled are already covered in
the four `if` branches.
2024-10-25 15:02:07 -04:00
Antoine Poinsot
38fdf7c1fb
mapport: drop outdated comments 2024-10-25 14:39:03 -04:00
merge-script
9a7206a34e
Merge bitcoin/bitcoin#29536: fuzz: fuzz connman with non-empty addrman + ASMap
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
552cae243a fuzz: cover `ASMapHealthCheck` in connman target (brunoerg)
33b0f3ae96 fuzz: use `ConsumeNetGroupManager` in connman target (brunoerg)
18c8a0945b fuzz: move `ConsumeNetGroupManager` to util (brunoerg)
fe624631ae fuzz: fuzz `connman` with a non-empty addrman (brunoerg)
0a12cff2a8 fuzz: move `AddrManDeterministic` to util (brunoerg)

Pull request description:

  ### Motivation

  Currently, we fuzz connman with an addrman from `NodeContext`. However,
  fuzzing connman with only empty addrman might not be effective, especially
  for functions like `GetAddresses` and other ones that plays with addrman. Also,
  we do not fuzz connman with ASMap, what would be good for functions that need
  `GetGroup`, or even for addrman. Without it, I do not see how effective would be
   fuzzing `ASMapHealthCheck`, for example.

  ### Changes

  - Move `AddrManDeterministic` and `ConsumeNetGroupManager` to util.
  - Use `ConsumeNetGroupManager` in connman target to construct a netgroupmanager
  and use it for `ConnmanTestMsg`.
  - Use `AddrManDeterministic` in connman target to create an addrman. It does
   not slow down as "filling" the addrman (e.g. with `FillAddrman`).
  - Add coverage for `ASMapHealthCheck`.

ACKs for top commit:
  maflcko:
    review ACK 552cae243a 🏀
  dergoegge:
    Code review ACK 552cae243a
  marcofleon:
    Code review ACK 552cae243a. Changes match the PR description.

Tree-SHA512: ba861c839602054077e4bf3649763eeb48357cda83ca3ddd32b02a1b61f4e44a0c5070182f001f9bf531d0d64717876279a7de3ddb9de028b343533b89233851
2024-10-25 15:18:54 +01:00
merge-script
d4abaf8c9d
Merge bitcoin/bitcoin#29608: optimization: Preallocate addresses in GetAddr based on nNodes
66082ca348 Preallocate addresses in GetAddr based on nNodes (Lőrinc)

Pull request description:

  The reserve method optimizes memory allocation by preallocating space for the expected number of elements (nNodes), reducing reallocations and improving performance. The upper bound ensures efficient memory usage based on the input constraints.

  before:
  ```
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |           76,852.79 |           13,011.89 |    0.4% |      1.07 | `AddrManGetAddr`
  |           76,598.21 |           13,055.14 |    0.2% |      1.07 | `AddrManGetAddr`
  |           76,296.32 |           13,106.79 |    0.1% |      1.07 | `AddrManGetAddr`
  ```

  after:
  ```
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |           65,966.97 |           15,159.10 |    0.3% |      1.07 | `AddrManGetAddr`
  |           66,075.40 |           15,134.23 |    0.2% |      1.06 | `AddrManGetAddr`
  |           66,306.34 |           15,081.51 |    0.3% |      1.06 | `AddrManGetAddr`
  ```

ACKs for top commit:
  stickies-v:
    ACK 66082ca348
  vasild:
    ACK 66082ca348

Tree-SHA512: 1175cff250d9c52ed042e8807ddc2afd64a806e6f2195b5c648752869ff3beec0be8a8cbd7ab6ba35cd7077d79b88a380da6c6e244f5549f98cdd472808b6d8f
2024-10-25 14:45:42 +01:00
Antoine Poinsot
b7b2435290
doc: add release note for #31130 2024-10-25 09:27:30 -04:00
Antoine Poinsot
1b6dec98da
depends: drop miniupnpc 2024-10-25 09:27:30 -04:00
Antoine Poinsot
953533d021
doc: remove mentions of UPnP 2024-10-25 09:27:29 -04:00
Antoine Poinsot
94ad614482
ci: remove UPnP options 2024-10-25 09:27:12 -04:00
Greg Sanders
f32c34d0c3 functional test: Additional package evaluation coverage
Current test coverage doesn't ensure that mempool trimming
doesn't appear prior to the entire package, and not just
the subpackage, is finished being submitted.

Add a scenario that covers this case, where package
ancestors can make it in individually, but would be
immadiately evicted if not for the package CPFP.
2024-10-25 09:22:57 -04:00
dergoegge
9f243cd7fa Introduce g_fuzzing global for fuzzing checks 2024-10-25 13:12:55 +01:00
merge-script
b95adf057a
Merge bitcoin/bitcoin#31150: util: Treat Assume as Assert when evaluating at compile-time
fa69a5f4b7 util: Treat Assume as Assert when evaluating at compile-time (MarcoFalke)

Pull request description:

  There is no downside or cost of treating an `Assume` at compile-time as an `Assert` and it may even help to find bugs while compiling without `ABORT_ON_FAILED_ASSUME`.

  This is also required for https://github.com/bitcoin/bitcoin/pull/31093

ACKs for top commit:
  dergoegge:
    ACK fa69a5f4b7
  brunoerg:
    ACK fa69a5f4b7
  marcofleon:
    ACK fa69a5f4b7

Tree-SHA512: 17604403f841343a6d5b6e5d777e1760d38e0c27dc1fd4479e3741894fba40cdb1fb659cf24519a51d051bd5884a75992d1227ec9fa40fbf53bc619fbfb304ad
2024-10-25 13:10:19 +01:00
merge-script
8f24e492e2
Merge bitcoin/bitcoin#29991: depends: sqlite 3.46.1
def6dd0c59 depends: sqlite 3.46.1 (fanquake)

Pull request description:

  Update sqlite in depends from [3.38.5](https://sqlite.org/releaselog/3_38_5.html) to [3.46.1](https://sqlite.org/releaselog/3_46_1.html).

ACKs for top commit:
  TheCharlatan:
    ACK def6dd0c59
  theuni:
    Not opposed utACK def6dd0c59

Tree-SHA512: 1f12c8ed8d05600b8240bcdbad5cf7d073ea5ab0bbd4a0f49a39ccfe1a93c043ee855b6eb0c67028edec57d8c21588dc33246e64d0b94feafad1a6ec38839893
2024-10-25 11:43:09 +01:00
merge-script
2ef5004f78
Merge bitcoin/bitcoin#31146: ci: Temporary workaround for old CCACHE_DIR cirrus env
fa9747a896 ci: Temporary workaround for old CCACHE_DIR cirrus env (MarcoFalke)

Pull request description:

  On a CI re-run, the historic env vars and CI config is used from Cirrus. However, the most recent CI config and CI scripts from this repo are used. This may lead to issues.

  For example, `CCACHE_DIR` in the old location may be missing on new CI workers and lead to errors.

  Fix it, by falling back to the old logic when the old `CCACHE_DIR` was detected.

ACKs for top commit:
  fanquake:
    ACK fa9747a896 - have seen this now.

Tree-SHA512: 04f0ca8d09ab0b8216a474fde1e05b79fbc6524884be173e8d728799739b026cda18d1797e0fe53d7e1b0ea69c0485acfe4f8a8f85408ea5bfdcffcf13a7ce55
2024-10-25 10:46:04 +01:00
merge-script
8c12fe828d
Merge bitcoin/bitcoin#29936: fuzz: wallet: add target for CreateTransaction
c495731a31 fuzz: wallet: add target for `CreateTransaction` (brunoerg)
3db68e29ec wallet: move `ImportDescriptors`/`FuzzedWallet` to util (brunoerg)

Pull request description:

  This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
  ```diff
  @@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
       // This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
       // and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
       if (selection_target == 0 && !coin_control.HasSelected()) {
  -        return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
  +       // return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
       }
  ```

  Also, it moves `ImportDescriptors` function to `src/wallet/test/util.h` to avoid to duplicate same code.

ACKs for top commit:
  marcofleon:
    ACK c495731a31
  maflcko:
    ACK c495731a31 🏻

Tree-SHA512: a439f947b91b01e327e18cd18e63d5ce49f2cb9ca16ca9d56fe337b8cff239b3af4db18fe89478fe5faa5549d37ca935bd321913db7646fbf6818f825cb5d878
2024-10-25 09:17:31 +01:00
kevkevinpal
5c299ecafe
test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped 2024-10-24 21:48:39 -04:00
Ava Chow
947f2925d5
Merge bitcoin/bitcoin#31124: util: Remove RandAddSeedPerfmon
Some checks are pending
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / test each commit (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
9bb92c0e7f util: Remove RandAddSeedPerfmon (Hodlinator)

Pull request description:

  `RegQueryValueExA(HKEY_PERFORMANCE_DATA, ...)` sometimes hangs *bitcoind.exe* on Windows during startup, at least on CI.

  We have other sources of entropy to seed randomness with on Windows, so should be alright removing this. Might resurrect if less drastic fix is found.

  Hopefully sufficient to fix #30390.

  CI debugged with temporary Windows stack trace dumping + Symbols in #30956.

ACKs for top commit:
  achow101:
    ACK 9bb92c0e7f
  fanquake:
    ACK 9bb92c0e7f
  hebasto:
    ACK 9bb92c0e7f, I have reviewed the code and it looks OK.
  laanwj:
    Code review ACK  9bb92c0e7f

Tree-SHA512: d3f26b4dd0519ef957f23abaffc6be1fed339eae756aed18042422fc6f0bba4e8fa9a44bf903e54f72747e2d0108146c18fd80576d95fc20690a2daf9c83689d
2024-10-24 18:08:12 -04:00
Ava Chow
7640cfdd62
Merge bitcoin/bitcoin#31118: doc: replace -? with -h and -help
33a28e252a Change default help arg to `-help` and mention `-h` and `-?` as alternatives (Lőrinc)
f0130ab1a1 doc: replace `-?` with `-h` for bench_bitcoin help (Lőrinc)

Pull request description:

  The question mark is interpreted as a wildcard for any single character in Zsh (see https://www.techrepublic.com/article/globbing-wildcard-characters-with-zsh), so `bench_bitcoin -?` will not show the help message on systems using Zsh, such as macOS.

  Since `-h` provides equivalent help functionality (as defined in https://github.com/bitcoin/bitcoin/blob/master/src/common/args.cpp#L684-L693), the `benchmarking.md` documentation has been updated to ensure compatibility with macOS.

  ----

  ### -?
  > % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -?
  zsh: no matches found: -?

  ### -h
  > % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -h
  Usage:  bench_bitcoin [options]
  Options:
  ...

  ----

  Based on the comments the args help default was also changed to `-help`, mentioning `-h` and `-?` (instead of `-?` being the default)

ACKs for top commit:
  edilmedeiros:
    tACK 33a28e252a
  maflcko:
    lgtm ACK 33a28e252a
  achow101:
    ACK 33a28e252a
  rkrux:
    tACK 33a28e252a
  laanwj:
    Code review ACK 33a28e252a

Tree-SHA512: 8c6e27488462be9ba9186b34abe6249c1d93026b3963acc0f42c75496f39407563766ae518cf1839156039cc0047e29d91f70d191cfb97e0fbde85665e88c71e
2024-10-24 18:01:41 -04:00
Ava Chow
74fb19317a
Merge bitcoin/bitcoin#30849: refactor: migrate bool GetCoin to return optional<Coin>
4feaa28728 refactor: Rely on returned value of GetCoin instead of parameter (Lőrinc)
46dfbf169b refactor: Return optional of Coin in GetCoin (Lőrinc)
e31bfb26c2 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 4feaa28728
  achow101:
    ACK 4feaa28728
  laanwj:
    Code review ACK 4feaa28728
  theStack:
    Code-review ACK 4feaa28728

Tree-SHA512: 818d60b2e97f58c489a61120fe761fb67a08dffbefe7a3fce712d362fc9eb8c2cced23074f1bec55fe71c616a3561b5a8737919ad6ffb2635467ec4711683df7
2024-10-24 13:52:47 -04:00
Ava Chow
c16e909b3e
Merge bitcoin/bitcoin#28574: wallet: optimize migration process, batch db transactions
c98fc36d09 wallet: migration, consolidate external wallets db writes (furszy)
7c9076a2d2 wallet: migration, consolidate main wallet db writes (furszy)
9ef20e86d7 wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans' (furszy)
34bf0795fc wallet: refactor ApplyMigrationData to return util::Result<void> (furszy)
aacaaaa0d3 wallet: provide WalletBatch to 'RemoveTxs' (furszy)
57249ff669 wallet: introduce active db txn listeners (furszy)
91e065ec17 wallet: remove post-migration signals connection (furszy)
055c0532fc wallet: provide WalletBatch to 'DeleteRecords' (furszy)
122d103ca2 wallet: introduce 'SetWalletFlagWithDB' (furszy)
6052c7891d wallet: decouple default descriptors creation from external signer setup (furszy)
f2541d09e1 wallet: batch MigrateToDescriptor() db transactions (furszy)
66c9936455 bench: add coverage for wallet migration process (furszy)

Pull request description:

  Last step in a chain of PRs (#26836, #28894, #28987, #29403).

  #### Detailed Description:
  The current wallet migration process performs only individual db writes. Accessing disk to
  delete all legacy records, clone and clean each address book entry for every created wallet,
  create each new descriptor (with their corresponding master key, caches and key pool), and
  also clone and delete each transaction that requires to be transferred to a different wallet.

  This work consolidates all individual disk writes into two batch operations. One for the descriptors
  creation from the legacy data and a second one for the execution of the migration process itself.
  Efficiently dumping all the information to disk at once atomically at the end of each process.

  This represent a speed up and also a consistency improvement. During migration, we either
  want to succeed or fail. No other outcomes should be accepted. We should never leave a
  partially migrated wallet on disk and request the user to manually restore the previous wallet from
  a backup (at least not if we can avoid it).

  Since the speedup depends on the storage device, benchmark results can vary significantly.
  Locally, I have seen a 15% speedup on a USB 3.2 pendrive.

  #### Note for Testers:
  The first commit introduces a benchmark for the migration process. This one can be
  cherry-picked on top of master to compare results pre and post changes.

  Please note that the benchmark setup may take some time (~70 seconds here) due to the absence
  of a batching mechanism for the address generation process (`GetNewDestination()` calls).

ACKs for top commit:
  achow101:
    ACK c98fc36d09
  theStack:
    re-ACK c98fc36d09
  pablomartin4btc:
    re-ACK c98fc36d09

Tree-SHA512: a52d5f2eef27811045d613637c0a9d0b7e180256ddc1c893749d98ba2882b570c45f28cc7263cadd4710f2c10db1bea33d88051f29c6b789bc6180c85b5fd8f6
2024-10-24 13:30:47 -04:00
Antoine Poinsot
a9598e5eaa
build: drop miniupnpc dependency 2024-10-24 18:23:31 +02:00
Antoine Poinsot
a5fcfb7385
interfaces: remove now unused 'use_upnp' arg from 'mapPort' 2024-10-24 18:23:30 +02:00
Antoine Poinsot
038bbe7b20
daemon: remove UPnP support
Keep the "-upnp" option as a hidden arg for one major version in order
to show a more user friendly error to people who had this option set in
their config file.
2024-10-24 18:23:30 +02:00
Antoine Poinsot
844770b05e
qt: remove UPnP settings 2024-10-24 18:23:29 +02:00
merge-script
dd92911732
Merge bitcoin/bitcoin#31148: ci: display logs of failed unit tests automatically
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
8523d8c0fc ci: display logs of failed tests automatically (furszy)

Pull request description:

  Saw it here https://github.com/bitcoin/bitcoin/actions/runs/11488618084/job/31975712362?pr=31000.

  The 'test-each-commit' and 'win64' CI jobs currently do not display test logs when an error occurs, making it almost impossible to debug issues that don't arise locally. Fix this by setting the CTest `--output-on-failure` flag (per [README](2f40e453cc/src/test/README.md (L130))).

ACKs for top commit:
  hebasto:
    ACK 8523d8c0fc, I have reviewed the code and it looks OK.

Tree-SHA512: 59c025099fb623e2ed430cfc1ba808e1d3ff72773d021e2280a44423ae53615c16e96a07014eb8581c95aae241b6d2777e388a8931ff0904feb84ca45cb22763
2024-10-24 16:53:53 +01:00
MarcoFalke
fa69a5f4b7
util: Treat Assume as Assert when evaluating at compile-time 2024-10-24 17:18:46 +02:00
merge-script
0c79c343a9
Merge bitcoin/bitcoin#31147: cmake, qt, test: Remove problematic code
fb46d57d4e cmake, qt, test: Remove problematic code (Hennadii Stepanov)

Pull request description:

  Split from https://github.com/bitcoin/bitcoin/pull/30997.

  The removed code aimed to make Qt's minimal integration plugin DLL available for `test_bitcoin-qt.exe` on Windows.

  However, there are two issues:
  1. The code is broken because the destination directory must end with a trailing slash (`/`).
  2. It is unnecessary because Qt's minimal integration plugin is not used on Windows. For more details, please refer to the following code:fb46d57d4e/src/qt/test/CMakeLists.txt (L38-L44)

  As a side effect, this PR fixes https://github.com/bitcoin-core/gui/issues/842.

ACKs for top commit:
  fanquake:
    ACK fb46d57d4e
  TheCharlatan:
    ACK fb46d57d4e

Tree-SHA512: b44d1c5e352e9bbfbba3c263ee03838cd490435da0490d9c8a152e60515520772c8a87aca08d4510f50c2e46b64ac92c666fa81accf43758af2e896693c44ffa
2024-10-24 14:56:29 +01:00
furszy
8523d8c0fc
ci: display logs of failed tests automatically 2024-10-24 10:36:02 -03:00
merge-script
2f40e453cc
Merge bitcoin/bitcoin#29450: build: replace custom MAC_OSX macro with existing __APPLE__
6c6b2442ed build: Replace MAC_OSX macro with existing __APPLE__ (Lőrinc)

Pull request description:

  This PR aims to standardize and simplify macOS-specific checks within our codebase by replacing the custom-defined `MAC_OSX` macro with the existing `__APPLE__`macro, defined in e.g. https://sourceforge.net/p/predef/wiki/OperatingSystems/#macos

  We already use `__APPLE__` in our own codebase for e.g. https://github.com/bitcoin/bitcoin/blob/master/src/crypto/sha256.cpp#L22

  Local Verification confirms that `MAC_OSX` isn't defined, but `__APPLE__` is:
  ```bash
  % echo | cpp -dM | egrep 'MAC_OSX|__MACOS__|__APPLE__'
  #define __APPLE__ 1
  ```

ACKs for top commit:
  fanquake:
    ACK 6c6b2442ed - at this point it seems unlikely that we'll need to accomodate non-macOS Apple, so consolidating to `__APPLE__` seems ok for now.

Tree-SHA512: dbf87c96211d9d55426ee85d76ef1e05cda3efd1c9248b0974a82834dafc1c1aece3165bd46e4252f0460dc97079bdbcebe98bbd81e9de0d7399c0bc69d5c050
2024-10-24 13:46:12 +01:00
Lőrinc
6c6b2442ed build: Replace MAC_OSX macro with existing __APPLE__
Adopting `__APPLE__` aligns our project with broader industry practices, including those in prominent projects such as the Linux kernel (and even our own code).

See: https://sourceforge.net/p/predef/wiki/OperatingSystems/#macos
2024-10-24 12:29:26 +02:00
Hennadii Stepanov
fb46d57d4e
cmake, qt, test: Remove problematic code
The removed code aimed to make Qt's minimal integration plugin DLL
available for `test_bitcoin-qt.exe` on Windows.

However, there are two issues:
1. The code is broken because the destination directory must end with a
   trailing slash (`/`).
2. It is unnecessary because Qt's minimal integration plugin is not
   used on Windows. For more details, please refer to the following
   code.
2024-10-24 11:27:16 +01:00
MarcoFalke
fa9747a896
ci: Temporary workaround for old CCACHE_DIR cirrus env 2024-10-24 11:43:33 +02:00
merge-script
d94adc7270
Merge bitcoin/bitcoin#29702: fees: Remove CLIENT_VERSION serialization
fa1c5cc9df fees: Log non-fatal errors as [warning], instead of info-level (MarcoFalke)
ddddbac9c1 fees: Pin required version to 149900 (MarcoFalke)
fa5126adcb fees: Pin "version that wrote" to 0 (MarcoFalke)

Pull request description:

  Coupling the fees serialization with CLIENT_VERSION is problematic, because:

  * `CLIENT_VERSION` may change, even though the serialization format does not change. This is harmless, but still confusing.
  * If a serialization format change was backported (unlikely), it may lead to incorrect results.
  * `CLIENT_VERSION` is changed at a different time during the release process than any serialization format change. This is harmless for releases of Bitcoin Core, but may be confusing when using the development branch.
  * It is harder to reason about a global `CLIENT_VERSION` when changing the format, than to reason about a versioning local to the module.

  Fix all issues by pinning the current version number in the module locally. In the future it can then be modified locally to the module, if needed.

ACKs for top commit:
  hodlinator:
    re-ACK fa1c5cc9df
  TheCharlatan:
    Re-ACK fa1c5cc9df

Tree-SHA512: 93870176ed50cc5a734576d66398a6036b31632228a9e05db1fa5452229e35ba4126f003e7db246aeb9891764ed47bde4470c674ec2bce7fd3ddd97e43944627
2024-10-24 10:09:36 +01:00
merge-script
7290bc61c0
Merge bitcoin/bitcoin#31078: build: Fix kernel static lib component install
82e16e6983 cmake: Refactor install kernel dependencies (Hennadii Stepanov)
42e6277987 build: Add static libraries to Kernel install component (TheCharlatan)

Pull request description:

  Fixes the installation of the pkgconfig file and the static library when installing only the `Kernel` component.

  This is a followup to fix #30835 and #30814, which were merged shortly after one another, but are interrelated. Can be tested with:
  ```
  cmake -B build -DBUILD_SHARED_LIBS=OFF -DBUILD_KERNEL_LIB=ON
  cmake --build build --target bitcoinkernel
  cmake --install build --component Kernel
  ```

ACKs for top commit:
  hebasto:
    ACK 82e16e6983, tested on Ubuntu 23.10.
  fanquake:
    ACK 82e16e6983

Tree-SHA512: 07c18a341d4464e489c28fb262600338f1711248309ffb2af0ef3ab1abf06f10873c435895b63010e0be8e44af77046324896dfd872479792aa049831606dc45
2024-10-24 09:55:19 +01:00
merge-script
68f29b2490
Merge bitcoin/bitcoin#31141: doc: Make list of targets in depends README consistent
a0c9595810 doc: Make list of targets in depends README consistent (laanwj)

Pull request description:

  The description of `i686-pc-linux-gnu` and `x86_64-pc-linux-gnu` is incomplete and inconsistent with the others. Fix this. Also use "64 bit" consistently instead of "64-bit".

ACKs for top commit:
  maflcko:
    lgtm ACK a0c9595810
  hebasto:
    ACK a0c9595810.
  jarolrod:
    ACK a0c9595810
  rkrux:
    ACK a0c9595810

Tree-SHA512: eedefb19639dd08f25627ceaab0d2c3745b256e561e55f8506d14721d0236978f1b1bef79f9fe126b7f42d869887ca988d04b3536d98a27e0eb182f0a7f64183
2024-10-24 09:10:59 +01:00
Ava Chow
e9b95665ee
Merge bitcoin/bitcoin#31046: init: Some small chainstate load improvements
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
31cc5006c3 init: Return fatal failure on snapshot validation failure (Martin Zumsande)
8f1246e833 init: Improve chainstate init db error messages (TheCharlatan)
cd093049dd init: Remove incorrect comment about shutdown condition (MarcoFalke)
635e9f85d7 init: Remove misleading log line when user chooses not to retry (TheCharlatan)
720ce880a3 init: Improve comment describing chainstate load retry behaviour (Martin Zumsande)
baea842ff1 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 31cc5006c3
  mzumsande:
    Code Review / lightly tested ACK 31cc5006c3
  BrandonOdiwuor:
    Code Review ACK 31cc5006c3.
  stickies-v:
    ACK 31cc5006c3

Tree-SHA512: 59fba4845ee45a3d91bf55807ae6b1c81458463b96bf664c8b1badfac503f6b01efd52a915fc399294e68a3f69985362a5a10a3844fa23f7707145ebe9ad349b
2024-10-23 18:33:31 -04:00
Ava Chow
b8c821cc1e
Merge bitcoin/bitcoin#30724: test: add test for specifying custom pidfile via -pid
04e4d52420 test: add test for specifying custom pidfile via `-pid` (Sebastian Falbesoner)
b832ffe044 refactor: introduce default pid file name constant in tests (tdb3)

Pull request description:

  This small PR adds test coverage for the `-pid` command line option, which allows to overrule the pid filename (`bitcoind.pid` by default). One can specify either a relative path (within the datadir) or an absolute one; the latter is tested using `self.options.tmpdir`. Note that the functional test file `feature_init.py` so far only contained a stress test; with this new sub-test added, both the description and the test name are adapted to be more generic.

ACKs for top commit:
  achow101:
    ACK 04e4d52420
  tdb3:
    ACK 04e4d52420
  ryanofsky:
    Code review ACK 04e4d52420
  naiyoma:
    Tested ACK [04e4d52420)

Tree-SHA512: b2bc8a790e5d187e2c84345f344f65a176b62caecd9797c3b9edf10294c741c33a24e535be640b56444b91dcf9c65c7dd152cdffd8b1c1d9ca68e5e3c6ad1e99
2024-10-23 17:39:30 -04:00
Martin Zumsande
6c9fe7b73e test: Prevent connection attempts to random IPs in p2p_seednodes.py
These addrs aren't unreachable as the test claims.
Specify a (non-working) proxy to make sure the connections fails
even if the addr was reachable.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
2024-10-23 15:39:48 -04:00