Commit graph

41483 commits

Author SHA1 Message Date
Martin Zumsande
de71d4dece fuzz: improve utxo_snapshot target
Add the possibility of giving more guidance to the creation of the
metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes
successfully creates a valid snapshot.

This also changes the asserts for the success case that were outdated,
and only didn't result in a crash because the fuzzer wasn't able
to reach this code before.
2024-07-04 20:12:47 -04:00
Ava Chow
173ab0ccf2
Merge bitcoin/bitcoin#29720: rpc: Avoid getchaintxstats invalid results
2342b46c45 test: Add coverage for getchaintxstats in assumeutxo context (Fabian Jahr)
faf2a6750b rpc: Reorder getchaintxstats output (MarcoFalke)
fa2dada0c9 rpc: Avoid getchaintxstats invalid results (MarcoFalke)

Pull request description:

  The `getchaintxstats` RPC reply during AU background download may return non-zero, but invalid, values for `window_tx_count` and `txrate`.

  For example, `txcount` may be zero for a to-be-downloaded block, but may be non-zero for an ancestor block which is already downloaded. Thus, the values returned may be negative (and cause intermediate integer sanitizer violations).

  Also, `txcount` may be accurate for the snapshot base block, or a descendant of it. However it may be zero for an ancestor block that still needs to be downloaded. Thus, the values returned may be positive, but wrong.

  Fix all issues by skipping the returned value if either `txcount` is unset (equal to zero).
  Also, skip `txcount` in the returned value, if it is unset (equal to zero).

  Fixes https://github.com/bitcoin/bitcoin/issues/29328

ACKs for top commit:
  fjahr:
    re-ACK 2342b46c45
  achow101:
    ACK 2342b46c45
  mzumsande:
    ACK 2342b46c45

Tree-SHA512: 931cecc40ee5dc0f96be728db7eb297155f8343076cd29c8b8c050c99fd1d568b80f54c9459a34ca7a9489c2474c729796d00eeb1934d6a9f7b4d6a53e3ec430
2024-07-02 18:02:26 -04:00
Ava Chow
3325a0afa4
Merge bitcoin/bitcoin#30272: doc: use TRUC instead of v3 and add release note
926b8e39dc [doc] add release note for TRUC (glozow)
19a9b90617 use version=3 instead of v3 in debug strings (glozow)
881fac8e60 scripted-diff: change names from V3 to TRUC (glozow)
a573dd2617 [doc] replace mentions of v3 with TRUC (glozow)
089b5757df rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89 rename policy/v3_policy.* to policy/truc_policy.* (glozow)

Pull request description:

  Adds a release note for TRUC policy which will be live in v28.0.

  For clarity, replaces mentions of "v3" with "TRUC" in most places. Suggested in
  - https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583
  - https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1624500904

  I changed error strings from "v3-violation" to "TRUC-violation" but left v3 in the debug strings because I think it might be clearer for somebody who is debugging. Similarly, I left some variables unchanged because I think they're more descriptive this way, e.g. `tx_v3_from_v2_and_v3`. I'm happy to debate places that should or shouldn't be documented differently in this PR, whatever is clearest to everyone.

ACKs for top commit:
  instagibbs:
    reACK 926b8e39dc
  achow101:
    ACK 926b8e39dc
  ismaelsadeeq:
    Code review ACK 926b8e39dc

Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
2024-07-02 17:49:32 -04:00
Ava Chow
9251bc7111
Merge bitcoin/bitcoin#30267: assumeutxo: Check snapshot base block is not in invalid chain
2f9bde69f4 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407e assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c0118 refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr)

Pull request description:

  This was discovered in a discussion in #29996

  If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.

  While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.

  The behavior change described above is in the second commit.

  The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`.

  The third commit removes an unnecessary restart introduced in #29428.

ACKs for top commit:
  mzumsande:
    re-ACK 2f9bde6
  alfonsoromanz:
    Re-ACK 2f9bde69f4. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
  achow101:
    ACK 2f9bde69f4

Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
2024-07-02 17:06:39 -04:00
Ava Chow
74d61151e5
Merge bitcoin/bitcoin#30365: #27307 follow-up: update mempool conflict tests + docs
7d55796c53 wallet: update mempool conflicts tests + docs (ishaanam)

Pull request description:

  #27307 follow-up:
  - updates description of `mempoolconflicts` and `walletconflicts` in `gettransaction`
  - adds release notes for 27307
  - removes unnecessary line from `wallet_conflicts.py`

ACKs for top commit:
  fjahr:
    ACK 7d55796c53
  achow101:
    ACK 7d55796c53
  furszy:
    utACK 7d55796c53
  tdb3:
    ACK 7d55796c53

Tree-SHA512: b3c368c7072cacdaf5fd18ecb0a88ab76ce02f65d56fce55a3316afa0989b9417c31e563aa8d9dd8f6294add154b4fdeb4ada5081c6b8a5fe9953f0e8a4812f4
2024-07-02 16:51:07 -04:00
Ava Chow
be63674c18
Merge bitcoin/bitcoin#30324: optimization: Moved repeated -printpriority fetching out of AddToBlock
323ce30308 Moved the repeated -printpriority fetching out of AddToBlock (Lőrinc)

Pull request description:

  `AddToBlock` was called repeatedly from `addPackageTxs` where the constant value of `printpriority` is recalculated every time.

  <img src="https://github.com/bitcoin/bitcoin/assets/1841944/6fd89647-7b6c-4f44-bd04-98d16cd2a938">

  This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a small speed increase for many iterations.

  > ./src/bench/bench_bitcoin --filter='AssembleBlock' --min-time=10000

  before:
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |          156,460.15 |            6,391.40 |    0.1% |     11.03 | `AssembleBlock`

  after:
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |          149,289.55 |            6,698.39 |    0.3% |     10.97 | `AssembleBlock`

  ---

  The slight speedup shows up in CI as well:
  <img src="https://github.com/bitcoin/bitcoin/assets/1841944/3be779c9-2dce-4a96-ae5f-cab5435bd72f">

ACKs for top commit:
  maflcko:
    ACK 323ce30308
  achow101:
    ACK 323ce30308
  tdb3:
    re ACK 323ce30308
  furszy:
    utACK 323ce30308

Tree-SHA512: c2a0aab429646453ad0470956529f1cac8c38778c4c53f82c92c6cbaaaeb69f3d3603c0014ff097844b151e9da7caa2371a4676244caea96527cb540e66825a3
2024-07-02 16:43:45 -04:00
Ava Chow
1e16b10cfa
Merge bitcoin/bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo
8ec24bdad8 test: Added coverage to Block not found error using gettxoutsetinfo (kevkevinpal)

Pull request description:

  #### Description
  There were no tests that checked for the `Block not found` error called in `ParseHashOrHeight` when using `gettxoutsetinfo`, this change adds coverage to it.

  You can see there are no tests that do the following by doing the below
  `grep -nri "Block not found.*gettxoutsetinfo" ./test/functional/`

  which leads to no results

ACKs for top commit:
  achow101:
    ACK 8ec24bdad8
  tdb3:
    ACK 8ec24bdad8
  kristapsk:
    ACK 8ec24bdad8
  brunoerg:
    crACK 8ec24bdad8
  alfonsoromanz:
    Re ACK 8ec24bdad8

Tree-SHA512: 2c61c681e7304c679cc3d7dd13af1b795780e85716c25c7423d68104e253d01271e048e21bc21be35dbc7ec1a4fde94e439542f3cfd669fe5a16478c5fa982ab
2024-07-02 16:35:25 -04:00
Ava Chow
6afc707c4f
Merge bitcoin/bitcoin#30339: test: add coverage for node field of getaddednodeinfo RPC
e38eadb2c2 test: change comments to `self.log.info` for `test_addnode_getaddednodeinfo` (brunoerg)
c838e3b610 test: add coverage for `node` field of `getaddednodeinfo` RPC (brunoerg)

Pull request description:

  We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.

ACKs for top commit:
  kevkevinpal:
    ACK [e38eadb](e38eadb2c2)
  achow101:
    ACK e38eadb2c2
  tdb3:
    re ACK e38eadb2c2
  BrandonOdiwuor:
    Code Review ACK e38eadb2c2
  rkrux:
    tACK [e38eadb](e38eadb2c2)

Tree-SHA512: e9f768b7aa86e58b0b0ced089ead57040ff9a5204493da1ab99c8bc897b6dcdce7c856855f74c52010fceef19af1e12a39eee9f8f2e7294b42476b6f980fe754
2024-07-02 16:28:44 -04:00
glozow
926b8e39dc [doc] add release note for TRUC 2024-07-02 12:20:12 +01:00
glozow
19a9b90617 use version=3 instead of v3 in debug strings
Make it more clear to the user what we mean by v3.
2024-07-02 12:20:12 +01:00
glozow
881fac8e60 scripted-diff: change names from V3 to TRUC
-BEGIN VERIFY SCRIPT-
sed -i 's/SingleV3Checks/SingleTRUCChecks/g' $(git grep -l 'SingleV3Checks')
sed -i 's/PackageV3Checks/PackageTRUCChecks/g' $(git grep -l 'PackageV3Checks')
sed -i 's/PV3C/PTRUCC/g' src/policy/truc_policy.h
sed -i 's/V3_MAX_VSIZE/TRUC_MAX_VSIZE/g' $(git grep -l 'V3_MAX_VSIZE')
sed -i 's/V3_CHILD_MAX_VSIZE/TRUC_CHILD_MAX_VSIZE/g' $(git grep -l 'V3_CHILD_MAX_VSIZE')
sed -i 's/V3_DESCENDANT_LIMIT/TRUC_DESCENDANT_LIMIT/g' $(git grep -l 'V3_DESCENDANT_LIMIT')
sed -i 's/V3_ANCESTOR_LIMIT/TRUC_ANCESTOR_LIMIT/g' $(git grep -l 'V3_ANCESTOR_LIMIT')
sed -i 's/CheckMempoolV3Invariants/CheckMempoolTRUCInvariants/g' $(git grep -l 'CheckMempoolV3Invariants')
-END VERIFY SCRIPT-
2024-07-02 12:06:07 +01:00
glozow
a573dd2617 [doc] replace mentions of v3 with TRUC
Keep mentions of v3 in debug strings to help people who might not know
that TRUC is applied when version=3.
Also keep variable names in tests, as it is less verbose to keep v3 and v2.
2024-07-02 12:06:07 +01:00
glozow
089b5757df rename mempool_accept_v3.py to mempool_truc.py 2024-07-02 11:57:59 +01:00
glozow
d2c8d161b4
Merge bitcoin/bitcoin#30344: kernel: remove mempool_persist
f1478c0545 mempool: move LoadMempool/DumpMempool to node (Cory Fields)
6d242ff1e9 kernel: remove mempool_persist.cpp (Cory Fields)

Pull request description:

  DumpMempool/LoadMempool are not necessary for the kernel.

  Noticed while working on instantiated logging.

  I suppose these could have been left in on purpose, but I'm assuming it was probably just an oversight.

ACKs for top commit:
  TheCharlatan:
    Re-ACK f1478c0545
  glozow:
    ACK f1478c0545
  stickies-v:
    ACK f1478c0545

Tree-SHA512: 5825da0cf2e67470524eb6ebe397eb90755a368469a25f184df99ab935b3eb6d89eb802b41a6c3661e869bba3bbfa8ba9d95281bc75ebbf790ec5d9d1f79c66f
2024-07-02 10:25:25 +01:00
merge-script
04d63fb8c3
Merge bitcoin/bitcoin#30374: Revert "test: p2p: check that connecting to ourself leads to disconnect"
9ec2c53701 Revert "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)

Pull request description:

  As suggested in https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200786670, this PR reverts the recently  added test  #30362 that causes frequent CI failures. A TODO is added in the functional test file to re-add it later when the race condition is fixed.

ACKs for top commit:
  mzumsande:
    utACK 9ec2c53701
  brunoerg:
    utACK 9ec2c53701
  tdb3:
    ACK 9ec2c53701

Tree-SHA512: df211ab194dc47f2ff8192f3827382974db922ed9fa54bc44fac75de4edfb3af43c1340cd5434b15b0b573f7b0ddd4451a0bbbbd7deaf7f4244e4865b9d5977e
2024-07-02 09:41:00 +01:00
Fabian Jahr
2342b46c45 test: Add coverage for getchaintxstats in assumeutxo context 2024-07-02 08:47:24 +02:00
MarcoFalke
faf2a6750b
rpc: Reorder getchaintxstats output 2024-07-02 08:46:06 +02:00
MarcoFalke
fa2dada0c9
rpc: Avoid getchaintxstats invalid results 2024-07-02 08:46:02 +02:00
Sebastian Falbesoner
9ec2c53701 Revert "test: p2p: check that connecting to ourself leads to disconnect"
This reverts commit 5d2fb14baf and
adds a TODO to add it later again once the race condition is fixed.
2024-07-01 20:53:16 +02:00
ishaanam
7d55796c53 wallet: update mempool conflicts tests + docs 2024-07-01 12:27:43 -04:00
merge-script
fe70be5377
Merge bitcoin/bitcoin#30369: ci: Clear unused /msan/llvm-project
fa6beb8cfc ci: Clear unused /msan/llvm-project (MarcoFalke)

Pull request description:

  Could help to fix the `no space left on device` that are sometimes seen.

ACKs for top commit:
  theuni:
    utACK fa6beb8cfc

Tree-SHA512: 0bedf4b26eed842c7bfa2caeac4df578cdbb00a658e8d0037b8b7b90150d8a9d1b8140437d1cf40b50d82a9085ea50cf9a010764c4439b2a03a457d399191319
2024-07-01 17:20:17 +01:00
MarcoFalke
fa6beb8cfc
ci: Clear unused /msan/llvm-project 2024-07-01 15:51:51 +02:00
glozow
0bd2bd1efb
Merge bitcoin/bitcoin#30237: test: Add Compact Block Encoding test ReceiveWithExtraTransactions covering non-empty extra_txn
55eea003af test: Make blockencodings_tests deterministic (AngusP)
4c99301220 test: Add ReceiveWithExtraTransactions Compact Block receive test. (AngusP)
4621e7cc8f test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases (AngusP)

Pull request description:

  This test uses the `extra_txn` (`vExtraTxnForCompact`) vector of optional orphan/conflicted/etc. transactions to provide transactions to a PartiallyDownloadedBlock that are not otherwise present in the mempool, and check that they are used.

  This also covers a former nullptr deref bug that was fixed in #29752 (bf031a517c) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.

ACKs for top commit:
  marcofleon:
    Code review ACK 55eea003af. I ran the `blockencodings` unit test and no issues with the new test case.
  dergoegge:
    Code review ACK 55eea003af
  glozow:
    ACK 55eea003af

Tree-SHA512: d7909c212bb069e1f6184b26390a5000dcc5f2b18e49b86cceccb9f1ec4f874dd43bc9bc92abd4207c71dd78112ba58400042c230c42e93afe55ba51b943262c
2024-07-01 14:11:52 +01:00
merge-script
4c573e5718
Merge bitcoin/bitcoin#30306: fuzz: Improve stability for txorphan and mini_miner harnesses
e009bf681c Don't use iterator addresses in IteratorComparator (dergoegge)

Pull request description:

  See #29018.

  Stability for `txorphan` is now >90%. `mini_miner` needs further investigation, stability still low (although slightly improved by this PR) at ~62%.

ACKs for top commit:
  marcofleon:
    Tested ACK e009bf681c. Using afl++, stability for `txorphan` went from 82% to ~94% and for `mini_miner` it went from 84% to 97%. I ran them both using the corpora in qa-assets.
  glozow:
    utACK e009bf681c

Tree-SHA512: 6d0a20fd7ceedca8e702d8adde5fca500d8b0187147aee8d43b4e9eb5176dcacf60180f42a7158f037d18dbb27e479b6c069a0f3c912226505cbff5aa073a415
2024-07-01 12:11:27 +01:00
merge-script
c3b446a494
Merge bitcoin/bitcoin#30273: fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read
4d81b4de33 fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read (Vasil Dimov)
b51d75ea97 fuzz: simplify FuzzedSock::m_peek_data (Vasil Dimov)

Pull request description:

  Problem:

  If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be
  retrieved from the fuzz provider, saved in `m_peek_data` and returned
  to the caller (ok).

  If after this `FuzzedSock::Recv(M, 0)` is called where `M < N`
  then the first `M` bytes from `m_peek_data` would be returned
  to the caller (ok), but the remaining `N - M` bytes in `m_peek_data`
  would be discarded/lost (not ok). They must be returned by a subsequent
  `Recv()`.

  To resolve this, only remove the head `N` bytes from `m_peek_data`.

  ---

  This is a followup to https://github.com/bitcoin/bitcoin/pull/30211, more specifically:

  https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633199919
  https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633216366

ACKs for top commit:
  marcofleon:
    ACK 4d81b4de33. Tested this with the I2P fuzz target and there's no loss in coverage. I think overall this is an improvement in the robustness of `Recv` in `FuzzedSock`.
  dergoegge:
    Code review ACK 4d81b4de33
  brunoerg:
    utACK 4d81b4de33

Tree-SHA512: 73b5cb396784652447874998850e45899e8cba49dcd2cc96b2d1f63be78e48201ab88a76cf1c3cb880abac57af07f2c65d673a1021ee1a577d0496c3a4b0c5dd
2024-07-01 11:58:58 +01:00
merge-script
2f813154ef
Merge bitcoin/bitcoin#30358: scripted-diff: Log parameter interaction not thrice
fa1bc7c88b scripted-diff: Log parameter interaction not thrice (MarcoFalke)
fafb7875e1 doc: Fix outdated dev comment about logging (MarcoFalke)

Pull request description:

  Seems a bit overkill to log the words "parameter interaction" thrice, when at least once is enough. So do that.

  Before:

  ```
  2024-06-28T15:30:57Z [init.cpp:745] [InitParameterInteraction] InitParameterInteraction: parameter interaction: -connect or -maxconnections=0 set -> setting -dnsseed=0
  ```

  After:

  ```
  2024-06-28T15:47:27Z [init.cpp:745] [InitParameterInteraction] parameter interaction: -connect or -maxconnections=0 set -> setting -dnsseed=0

ACKs for top commit:
  paplorinc:
    ACK fa1bc7c88b
  fjahr:
    utACK fa1bc7c88b
  TheCharlatan:
    Nice, ACK fa1bc7c88b
  hodlinator:
    utACK fa1bc7c88b

Tree-SHA512: 83cd92e20dffa38737d4fd31764481284383e12671d9e4b33cfa496743c95c10921a113b1da2caafeb44fca3759a28a8e230df5e30c29fb55d5854ff1531382c
2024-07-01 11:56:23 +01:00
merge-script
b3c22e0c72
Merge bitcoin/bitcoin#30362: test: p2p: check that connecting to ourself leads to disconnect
5d2fb14baf test: p2p: check that connecting to ourself leads to disconnect (Sebastian Falbesoner)

Pull request description:

  This small PR adds test coverage for the scenario of connecting to ourself, leading to an immediate disconnect:
  2f6dca4d1c/src/net_processing.cpp (L3729-L3735)

  This logic has been first introduced by Satoshi in October 2009, together with a couple of other changes and a version bump to "v0.1.6 BETA" (see commit cc0b4c3b62).

ACKs for top commit:
  kevkevinpal:
    tACK [5d2fb14](5d2fb14baf)
  maflcko:
    ACK 5d2fb14baf
  fjahr:
    tACK 5d2fb14baf
  tdb3:
    ACK 5d2fb14baf

Tree-SHA512: 30fb8c82cef94701affeca386ecd59daa32231635fa770fe225feb69fdab2ffedbfa157edd563f65099ec209f2dafffc1154f7f9292c2ea68bbd114750904875
2024-07-01 10:32:52 +01:00
Lőrinc
323ce30308 Moved the repeated -printpriority fetching out of AddToBlock
AddToBlock was called repeatedly from `addPackageTxs` where the constant value of `printpriority` is recalculated every time.
Since its behavior was changed in 400b151, I've named the variable accordingly.

This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a measurable speed increase for many iterations.

> ./src/bench/bench_bitcoin --filter='AssembleBlock' --min-time=1000

before:
|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          155,558.97 |            6,428.43 |    0.1% |      1.10 | `AssembleBlock`

after:
|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          148,083.68 |            6,752.94 |    0.1% |      1.10 | `AssembleBlock`

Co-authored-by: furszy <mfurszy@protonmail.com>
2024-06-30 23:00:13 +02:00
kevkevinpal
8ec24bdad8
test: Added coverage to Block not found error using gettxoutsetinfo 2024-06-30 10:46:33 -04:00
Sebastian Falbesoner
5d2fb14baf test: p2p: check that connecting to ourself leads to disconnect
The "connect to ourself" detection logic has been first introduced
by Satoshi in October 2009, together with a couple of other changes
and a version bump to "v0.1.6 BETA" (see commit
cc0b4c3b62).
2024-06-29 00:30:14 +02:00
MarcoFalke
fa1bc7c88b
scripted-diff: Log parameter interaction not thrice
-BEGIN VERIFY SCRIPT-
 sed -i 's/LogPrintf("%s: \(parameter interaction: .*\)", __func__/LogInfo("\1"/g' ./src/init.cpp
-END VERIFY SCRIPT-
2024-06-28 17:46:00 +02:00
MarcoFalke
fafb7875e1
doc: Fix outdated dev comment about logging 2024-06-28 17:37:58 +02:00
Ryan Ofsky
2f6dca4d1c
Merge bitcoin/bitcoin#30335: Mining interface followups, reduce cs_main locking, test rpc bug fix
a74b0f93ef Have testBlockValidity hold cs_main instead of caller (Sjors Provoost)
f6dc6db44d refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost)
5fb2b70489 Drop unneeded lock from createNewBlock (Sjors Provoost)
75ce7637ad refactor: testBlockValidity make out argument last (Sjors Provoost)
83a9bef0e2 Add missing include for mining interface (Sjors Provoost)

Pull request description:

  Followups from #30200

  Fixes:
  - `std::unique_ptr` needs `#include <memory>` (noticed while working on #30332, which has fewer includes than its parent PR that I originally tested with)
  - Drop lock from createNewBlock that was spuriously added
  - Have testBlockValidity hold cs_main instead of caller (also fixes a race condition in test-only code)

  Refactor:
  - Use CHECK_NONFATAL to avoid single-use symbol (refactor)
  - move output argument `state` to the end of `testBlockValidity`, see https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647987176

ACKs for top commit:
  AngusP:
    Code Review ACK a74b0f93ef
  itornaza:
    Tested ACK a74b0f93ef
  ryanofsky:
    Code review ACK a74b0f93ef. Just new error string is added since last review, and a commit message was updated

Tree-SHA512: 805e133bb59303fcee107d6f02b3e2761396c290efb731a85e6a29ae56b4b1b9cd28ada9629e979704dcfd98cf35034e7e6b618e29923049eb1eca2f65630e41
2024-06-27 18:16:27 -04:00
Ryan Ofsky
d38dbaad98
Merge bitcoin/bitcoin#28167: init: Add option for rpccookie permissions (replace 26088)
73f0a6cbd0 doc: detail -rpccookieperms option (willcl-ark)
d2afa2690c test: add rpccookieperms test (willcl-ark)
f467aede78 init: add option for rpccookie permissions (willcl-ark)
7df03f1a92 util: add perm string helper functions (willcl-ark)

Pull request description:

  This PR picks up #26088 by aureleoules which adds a bitcoind launch option `-rpccookieperms` to set the file permissions of the cookie generated by bitcoin core.

  Example usage to make the generated cookie group-readable: `./src/bitcoind -rpccookieperms=group`.

  Accepted values for `-rpccookieperms` are `[owner|group|all]`. We let `fs::perms` handle platform-specific permissions changes.

ACKs for top commit:
  achow101:
    ACK 73f0a6cbd0
  ryanofsky:
    Code review ACK 73f0a6cbd0. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
  tdb3:
    cr ACK 73f0a6cbd0

Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11
2024-06-27 17:35:08 -04:00
Ava Chow
f0745d028e
Merge bitcoin/bitcoin#30050: refactor, wallet: get serialized size of CRecipients directly
a9c7300135 move-only: refactor CreateTransactionInternal (josibake)
adc6ab25bb wallet: use CRecipient instead of CTxOut (josibake)

Pull request description:

  Broken out from #28201

  ---

  In order to estimate fees properly, we need to know what the final serialized transaction size will be. This PR refactors `CreateTransactionInternal` to:

  * Get the serialized size directly from the `CRecipient`: this sets us up in a future PR to calculate the serialized size of silent payment `CTxDestinations` (see 797e21c8c1)
  * Use the new `GetSerializeSizeForRecipient` to move the serialize size calculation to *before* coin selection and the output creation to *after* coin selection: this also sets us up for silent payments sending in a future PR in that silent payments outputs cannot be created until after the inputs to the transaction have been selected

  Aside from the silent payments use case, I think this structure logically makes more sense. As a reminder, move-only commits are best reviewed with something like `git diff -w --color-moved=dimmed-zebra`

ACKs for top commit:
  S3RK:
    reACK a9c7300135
  achow101:
    ACK a9c7300135
  rkrux:
    tACK [a9c7300](a9c7300135)

Tree-SHA512: 412e1764b98f7428c8530c3a68f55e32063d6b66ab2ff613e1c7e12d49b049807cb60055cfe7f7e8ffe7ac7f0f9931427cbfd3efe7d4f97a5a0f6d1bf1aaac58
2024-06-27 13:59:46 -04:00
willcl-ark
73f0a6cbd0
doc: detail -rpccookieperms option
Co-authored-by: tdb3 <106488469+tdb3@users.noreply.github.com>
2024-06-27 15:08:27 +01:00
willcl-ark
d2afa2690c
test: add rpccookieperms test
Tests various perms on non-Windows OSes
2024-06-27 15:08:23 +01:00
willcl-ark
f467aede78
init: add option for rpccookie permissions
Add a bitcoind launch option `-rpccookieperms` to configure the file
permissions of the cookie on Unix systems.
2024-06-27 15:08:19 +01:00
willcl-ark
7df03f1a92
util: add perm string helper functions
PermsToSymbolicString will convert from fs::perms to string type
'rwxrwxrwx'.

InterpretPermString will convert from a user-supplied "perm string" such
as 'owner', 'group' or 'all, into appropriate fs::perms.
2024-06-27 14:55:10 +01:00
Sjors Provoost
a74b0f93ef
Have testBlockValidity hold cs_main instead of caller
The goal of interfaces is to eventually run in their own process,
so we can't use EXCLUSIVE_LOCKS_REQUIRED in their declaration.

However TestBlockValidaty will crash (in its call to ConnectBlock)
if the tip changes from under the proposed block.

Have the testBlockValidity implementation  hold the lock instead,
and non-fatally check for this condition.
2024-06-27 08:58:25 +02:00
Sjors Provoost
f6dc6db44d
refactor: use CHECK_NONFATAL to avoid single-use symbol 2024-06-27 08:58:24 +02:00
Sjors Provoost
5fb2b70489
Drop unneeded lock from createNewBlock
This was added in 4bf2e361da, but
BlockAssembler::CreateNewBlock already locks cs_main internally.
2024-06-27 08:56:20 +02:00
Cory Fields
f1478c0545 mempool: move LoadMempool/DumpMempool to node 2024-06-26 22:47:09 +00:00
Ava Chow
b27afb7fb7
Merge bitcoin/bitcoin#29833: i2p: fix and improve logs
7d3662fbe3 i2p: fix log when an interruption happens during `Accept` (brunoerg)
3d3a83fab2 i2p: log errors properly according to their severity (brunoerg)

Pull request description:

  This PR improves and fixes i2p logs (joint work with vasild).

  - It replaces `LogPrint` to `LogPrintLevel` so we can log according to the severity.
  - Fix log when interruption happens during `Accept`. Before this PR, when an interruption happens, it just logs "Error accepting:", no reason is logged as it does for other situations. This PR changes it to log "Accept interrupted".
  - Log errors according to the severity. Stuff like creating SAM session, destroying SAM session, etc... are logged as 'debug'.

ACKs for top commit:
  achow101:
    ACK 7d3662fbe3
  marcofleon:
    ACK 7d3662fbe3.
  vasild:
    ACK 7d3662fbe3

Tree-SHA512: 1c3d92108dbc22833f37a78e18b4efd723433d10f28166d17c74eab884cd97e908b4e0a0908fd16288df895eb2eb480f781de37b2ec6a6d414abfb71e0c86fe2
2024-06-26 15:28:26 -04:00
Cory Fields
6d242ff1e9 kernel: remove mempool_persist.cpp
DumpMempool/LoadMempool are not necessary for the kernel
2024-06-26 18:58:54 +00:00
Ava Chow
9ac4f69ec2
Merge bitcoin/bitcoin#30334: Update libsecp256k1 subtree to latest master
1408944d2e Squashed 'src/secp256k1/' changes from 06bff6dec8..4af241b320 (fanquake)

Pull request description:

  Updates the libsecp256k1 subtree to f473c959f0. This includes a number of CMake related changes, including one that prevents CMake from segfaulting when we were configuring the subtree. A number of these changes have come from the review/discussion in https://github.com/hebasto/bitcoin/pull/192:

  * https://github.com/bitcoin-core/secp256k1/pull/1529
  * https://github.com/bitcoin-core/secp256k1/pull/1532
  * https://github.com/bitcoin-core/secp256k1/pull/1535
  * https://github.com/bitcoin-core/secp256k1/pull/1543
  * https://github.com/bitcoin-core/secp256k1/pull/1545
  * https://github.com/bitcoin-core/secp256k1/pull/1546

  Also includes:

  * https://github.com/bitcoin-core/secp256k1/pull/1488
  * https://github.com/bitcoin-core/secp256k1/pull/1517
  * https://github.com/bitcoin-core/secp256k1/pull/1533
  * https://github.com/bitcoin-core/secp256k1/pull/1548
  * https://github.com/bitcoin-core/secp256k1/pull/1550

ACKs for top commit:
  achow101:
    ACK cc58e958f3
  TheCharlatan:
    ACK cc58e958f3
  hebasto:
    re-ACK cc58e958f3.
  real-or-random:
    utACK cc58e958f3

Tree-SHA512: 41409bc7f65bd17a9feb5c0455e2de2d291a25e4ce14e4a01fe25fcf9d45c64ddf55f274c17d1c86a63ab6b4870997ab79c65ec2795e5b3b49502823770c500f
2024-06-26 12:37:28 -04:00
Ava Chow
1d00601b9b
Merge bitcoin/bitcoin#30309: wallet: notify when preset + automatic inputs exceed max weight
72b226882f wallet: notify when preset + automatic inputs exceed max weight (furszy)

Pull request description:

  Small change. Found it while finishing my review on #29523. This does not interfere with it.

  Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
  This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.

ACKs for top commit:
  achow101:
    ACK 72b226882f
  tdb3:
    re ACK for 72b226882f
  rkrux:
    tACK [72b2268](72b226882f)
  ismaelsadeeq:
    utACK 72b226882f

Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
2024-06-26 12:16:16 -04:00
merge-script
d3d2bbf576
Merge bitcoin/bitcoin#30327: build: Drop redundant sys/sysctl.h header check
c0b5ea5901 build: Drop redundant `sys/sysctl.h` header check (Hennadii Stepanov)

Pull request description:

  The `AC_CHECK_HEADERS` macro defines `HAVE_SYS_SYSCTL_H` if the `sys/sysctl.h` header is found. However, in the source code, this header is guarded by `HAVE_SYSCTL` and `HAVE_SYSCTL_ARND` macros, which have their own checks. Since `HAVE_SYS_SYSCTL_H` is not used, we can skip the `AC_CHECK_HEADERS(... sys/sysctl.h ...)` check.

ACKs for top commit:
  laanwj:
    ACK c0b5ea5901
  fanquake:
    ACK c0b5ea5901 - we could got the other way, and add nested #defs, but that doesn't seem worthwhile.

Tree-SHA512: 73bc4bbfc5c457cd2c38e40f8e57d2a70c06ef661d76d4148d683d262be45b9405b8cda1958ac611c312ca7d9e2f9624cf2cac1b61f1008af0856875c62f0eac
2024-06-26 15:25:46 +01:00
merge-script
0c57a798b5
Merge bitcoin/bitcoin#29987: guix: build with glibc 2.31
b5fc6d46a3 guix: use glibc 2.31 (fanquake)

Pull request description:

  Set minimum required glibc to 2.31.
  The glibc 2.31 branch is still maintained: https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/release/2.31/master.

  Remove the stack-protector check from test-security-check, as the test
  no-longer fails, and given the control we have of the end, the actual
  security-check test seems sufficient (this might also be applied to some
  of the other checks).

  Drops runtime support for Ubuntu Bionic 18.04 and RHEL-8 from the release binaries.

ACKs for top commit:
  TheCharlatan:
    ACK b5fc6d46a3

Tree-SHA512: ba7e727240fa0ebebfb8b749024c71cbfdec37c33b39627866d78f9318ccdc687fd5103a63ee0e98cf809d9954dde56b1b305691c33d1de275ed0519f716c921
2024-06-26 15:21:44 +01:00
merge-script
3f0ee7655b
Merge bitcoin/bitcoin#30007: chainparams: Add achow101 DNS seeder
2721d64989 chainparams: Add achow101 DNS seeder (Ava Chow)

Pull request description:

  I wrote a [DNS seeder](https://github.com/achow101/dnsseedrs) and have been running it for the past 2 months now. I believe it is ready/good enough to be used as an additional DNS seeder for all of our supported public networks.

ACKs for top commit:
  laanwj:
    ACK 2721d64989
  1440000bytes:
    ~~reACK 2721d64989~~
  mzumsande:
    ACK 2721d64989
  willcl-ark:
    reACK 2721d64989

Tree-SHA512: 857a6cf7dd33962f0008a89db4d6b57d3c6aa622704cdcca6ab710babeead3a2970d9a6fa190949c7bbf7cb7d006e814d6314be3d8c8180eed29013c7c1ac7e1
2024-06-26 11:43:58 +01:00