Commit Graph

40515 Commits

Author SHA1 Message Date
glozow d1e9a02126
Merge bitcoin/bitcoin#29402: mempool: Log added for dumping mempool transactions to disk
4d5b55735b log: renamed disk to file so wording was more accurate (kevkevin)
b9f04be870 mempool: Log added for dumping mempool transactions to disk (kevkevin)

Pull request description:

  Sometimes when shutting off bitcoind it can take a while to dump the mempool transaction onto the disk so
  this change adds additional logging to the `DumpMempool` method in `kernel/mempool_persist.cpp`

  Motivated by https://github.com/bitcoin/bitcoin/pull/29227 this change
   - adds a single new line for the amount of transactions being dumped and the amount of memory being dumped to file

  This is in response to https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1893375082

  The logs will now look like this
  ```
  2024-02-09T23:41:52Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.02s)
  2024-02-09T23:41:52Z scheduler thread exit
  2024-02-09T23:41:52Z Writing 29 mempool transactions to file...
  2024-02-09T23:41:52Z Writing 0 unbroadcast transactions to file.
  2024-02-09T23:41:52Z Dumped mempool: 0.000s to copy, 0.022s to dump, 0.015 MB dumped to file
  2024-02-09T23:41:52Z Flushed fee estimates to fee_estimates.dat.
  2024-02-09T23:41:53Z Shutdown: done
  ```

ACKs for top commit:
  maflcko:
    cr-ACK 4d5b55735b
  glozow:
    reACK 4d5b557

Tree-SHA512: 049191e140d00c1ea57debe0138f1c9eb0f9bb0ef8138e2568e6d89e64f45a5d5853ce3b9cc0b28566aab97555b47ddfb0f9199fc8cea6b81e53f50592d5ae6a
2024-03-28 11:43:10 +00:00
Ryan Ofsky c8e3978114
Merge bitcoin/bitcoin#27307: wallet: track mempool conflicts with wallet transactions
5952292133 wallet, rpc: show mempool conflicts in `gettransaction` result (ishaanam)
54e07ee22f wallet: track mempool conflicts (ishaanam)
d64922b590 wallet refactor: use CWalletTx member functions to determine tx state (ishaanam)
ffe5ff1fb6 scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted (ishaanam)
180973a941 test: Add tests for wallet mempool conflicts (ishaanam)

Pull request description:

  The `mempool_conflicts` variable is added to `CWalletTx`, it is a set of txids of txs in the mempool conflicting with the wallet tx or a wallet tx's parent. This PR only changes how mempool-conflicted txs are dealt with in memory.

  `IsSpent` now returns false for an output being spent by a mempool conflicted transaction where it previously returned true.

  A txid is added to `mempool_conflicts` during  `transactionAddedToMempool`. A txid is removed from `mempool_conflicts` during  `transactionRemovedFromMempool`.

  This PR also adds a `mempoolconflicts` field to the `gettransaction` wallet RPC result.

  Builds on #27145
  Second attempt at #18600

ACKs for top commit:
  achow101:
    ACK 5952292133
  ryanofsky:
    Code review ACK 5952292133. Just small suggested changes since last review
  furszy:
    ACK 59522921

Tree-SHA512: 615779606723dbb6c2e302681d8e58ae2052ffee52d721ee0389746ddbbcf4b4c4afacf01ddf42b6405bc6f883520524186a955bf6b628fe9b3ae54cffc56a29
2024-03-27 12:45:08 -04:00
fanquake 7a12cbed99
Merge bitcoin/bitcoin#29747: depends: fix mingw-w64 Qt DEBUG=1 build
b7e7e727ab depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)

Pull request description:

  The issue is that compilation is done with `x86_64-w64-mingw32-g++-posix`, but then linking is done with `x86_64-w64-mingw32-g++`.

  I'm guessing this has been broken since #24131 (01d1845a80), but have not checked.

  Fixes #29734.
  Unblocks #29527 (`DEBUG=1` builds can be tested).

ACKs for top commit:
  hebasto:
    ACK b7e7e727ab, tested on Ubuntu 22.04 with the [installed](https://github.com/bitcoin/bitcoin/issues/29734#issuecomment-2022852344) `g++-mingw-w64-x86-64` package.
  TheCharlatan:
    ACK b7e7e727ab

Tree-SHA512: 9e24e84046c0489c20971bb9c68d1a643c233837193c184f61bff79dfc8d7397a5c5526ac1a205ad423920f2589559cd01cb104ceb7f89515bb6421510d82ca9
2024-03-27 16:36:14 +00:00
kevkevin 4d5b55735b
log: renamed disk to file so wording was more accurate 2024-03-27 07:19:46 -05:00
kevkevin b9f04be870
mempool: Log added for dumping mempool transactions to disk 2024-03-27 07:18:49 -05:00
fanquake 28f2ca675f
Merge bitcoin/bitcoin#29479: test: Refactor subtree exclusion in lint tests
80fa7da21c test: Refactor subtree exclusion in lint tests (Brandon Odiwuor)

Pull request description:

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

  Refactor subtree exclusion in lint tests to one place

  Second attempt after PR: https://github.com/bitcoin/bitcoin/pull/24435

ACKs for top commit:
  fjahr:
    re-ACK 80fa7da21c
  maflcko:
    lgtm ACK 80fa7da21c
  davidgumberg:
    ACK 80fa7da21c

Tree-SHA512: deff7457dd19ca5ea440d3d53feae047e8863b9ddeb6494a3c94605a5d16edc91db8f99a435b4fab2ef89aedee42439562be006da647fb85bbf3def903a3ce50
2024-03-27 11:40:18 +00:00
fanquake b8b0d64765
Merge bitcoin/bitcoin#29740: ci: Print tsan errors to stderr
fa22a438fa ci: Print tsan errors to stderr (MarcoFalke)

Pull request description:

  This fixes a bug introduced in https://github.com/bitcoin/bitcoin/pull/27667

  All sanitizers print their errors to stderr, except for tsan, which prints to a file and expects the file to be read.

  Fix this by not using a log file in any sanitizer.

ACKs for top commit:
  dergoegge:
    utACK fa22a438fa

Tree-SHA512: 15dca57932a21bda145335fab6367bbf2ae67b25e0b7b61044d2c06ab7a8db3a452f057f6656b81a031726375b7bb238f5ced18ab8894f005e7ab254c7d1ef06
2024-03-27 11:07:02 +00:00
fanquake b7e7e727ab
depends: fix mingw-w64 Qt DEBUG=1 build
The issue is that compilation is done with `x86_64-w64-mingw32-g++-posix`,
but then linking is done with `x86_64-w64-mingw32-g++`.

I'm guessing this has been broken since #24131
(01d1845a80), but have not checked.

Fixes #29734.
Unblocks #29527 (now DEBUG=1 builds can be tested).
2024-03-27 10:52:32 +00:00
MarcoFalke fa22a438fa
ci: Print tsan errors to stderr 2024-03-26 19:19:28 +01:00
fanquake d04324a705
Merge bitcoin/bitcoin#29695: guix: build GCC with --enable-standard-branch-protection
7850c5fe20 guix: build GCC with --enable-standard-branch-protection (fanquake)

Pull request description:

  This is one change extracted from #24123 (which now produces fully BTI & PAC enabled bins), which will mean that everything in depends, for Guix builds, is compiled using `-mbranch-protection=standard`.

  Turning this on by default, is similar to what we already do with `--enable-default-ssp`, `--enable-default-pie` etc.

  See: https://gcc.gnu.org/install/specific.html#aarch64-x-x

  > To enable Branch Target Identification Mechanism and Return Address Signing by default at configure time use the `--enable-standard-branch-protection` option.

  > This is equivalent to having `-mbranch-protection=standard` during compilation. This can be explicitly disabled during compilation by passing the `-mbranch-protection=none` option which turns off all types of branch protections.

ACKs for top commit:
  TheCharlatan:
    ACK 7850c5fe20

Tree-SHA512: 18f898da27021bab502e708ea5fa9b325352f8f6e23d9488a2a0feda87e0af2ac0e4f87b3af9ad6a9a37bbfc99ab0285de4f0bdc174dcd38163d92c122e958e2
2024-03-26 13:45:33 +00:00
Brandon Odiwuor 80fa7da21c test: Refactor subtree exclusion in lint tests 2024-03-26 13:49:47 +03:00
glozow 19b968f743
Merge bitcoin/bitcoin#29722: 28950 followups
7b29119d79 use const ref for client_maxfeerate (Greg Sanders)
f10fd07320 scripted-diff: Rename max_sane_feerate to client_maxfeerate (Greg Sanders)

Pull request description:

  Follow-ups to https://github.com/bitcoin/bitcoin/pull/28950

ACKs for top commit:
  glozow:
    utACK 7b29119d79
  stickies-v:
    ACK 7b29119d79

Tree-SHA512: b9e13509c6e9d7c08aa9d4e759f9707004c1c7b8f3e521fe2ec0037160b87c7fb02528966b9f26eaca6291621df9300e84b5aec66dbc2e97d13bf2f3cd7f979c
2024-03-26 08:56:44 +00:00
glozow c2dbbc35b9
Merge bitcoin/bitcoin#29242: Mempool util: Add RBF diagram checks for single chunks against clusters of size 2
7295986778 Unit tests for CalculateFeerateDiagramsForRBF (Greg Sanders)
b767e6bd47 test: unit test for ImprovesFeerateDiagram (Greg Sanders)
7e89b659e1 Add fuzz test for FeeFrac (Greg Sanders)
4d6528a3d6 fuzz: fuzz diagram creation and comparison (Greg Sanders)
e9c5aeb11d test: Add tests for CompareFeerateDiagram and CheckConflictTopology (Greg Sanders)
588a98dccc fuzz: Add fuzz target for ImprovesFeerateDiagram (Greg Sanders)
2079b80854 Implement ImprovesFeerateDiagram (Greg Sanders)
66d966dcfa Add FeeFrac unit tests (Greg Sanders)
ce8e22542e Add FeeFrac utils (Greg Sanders)

Pull request description:

  This is a smaller piece of https://github.com/bitcoin/bitcoin/pull/28984 broken off for easier review.

  Up to date explanation of diagram checks are here: https://delvingbitcoin.org/t/mempool-incentive-compatibility/553

  This infrastructure has two near term applications prior to cluster mempool:
  1) Limited Package RBF(https://github.com/bitcoin/bitcoin/pull/28984): We want to allow package RBF only when we know it improves the mempool. This narrowly scoped functionality allows use with v3-like topologies, and will be expanded at some point post-cluster mempool when diagram checks can be done efficiently against bounded cluster sizes.
  2) Replacement for single tx RBF(in a cluster size of up to two) against conflicts of up to cluster size two. `ImprovesFeerateDiagram` interface will have to change for this use-case, which is a future direction to solve certain pins and improve mempool incentive compatibility: https://delvingbitcoin.org/t/ephemeral-anchors-and-mev/383#diagram-checks-fix-this-3

  And longer-term, this would be the proposed way we would compute incentive compatibility for all conflicts, post-cluster mempool.

ACKs for top commit:
  sipa:
    utACK 7295986778
  glozow:
    code review ACK 7295986778
  murchandamus:
    utACK 7295986778
  ismaelsadeeq:
    Re-ACK 7295986778
  willcl-ark:
    crACK 7295986778
  sdaftuar:
    ACK 7295986778

Tree-SHA512: 79593e5a087801c06f06cc8b73aa3e7b96ab938d3b90f5d229c4e4bfca887a77b447605c49aa5eb7ddcead85706c534ac5eb6146ae2396af678f4beaaa5bea8e
2024-03-26 08:48:37 +00:00
Ava Chow b44f9e4645
Merge bitcoin/bitcoin#28928: test: add coverage for bech32m in `wallet_keypool_topup`
a8bfc3dea1 test: add coverage for bech32m in `wallet_keypool_topup` (brunoerg)

Pull request description:

  0dcac51049 added coverage for all keypool addresses types in `wallet_keypool_topup` (4y ago). Now we have bech23m, so this PR adds it.

ACKs for top commit:
  achow101:
    ACK a8bfc3dea1
  marcofleon:
    ACK a8bfc3dea1. Definitely a more straightfoward addition to the test. Reviewed the code, built the PR branch and ran all functional tests without issues.
  furszy:
    utACK a8bfc3dea

Tree-SHA512: aa830b723a7a54b23744f9fb3cf5214452c4ffc8e3bbe0e8bd980bdf902e61c3dd2fd57361b82c5c0c5224aa0774158daf34b6b2188edda0a971f82111976051
2024-03-25 17:49:02 -04:00
fanquake 2102c978b5
Merge bitcoin/bitcoin#29706: depends: set two CMake options globally
76045bb9d6 depends: always set CMAKE_POSITION_INDEPENDENT_CODE=ON (fanquake)
d04623678c depends: always set CMAKE_INSTALL_LIBDIR=lib/ (fanquake)

Pull request description:

  Set `CMAKE_INSTALL_LIBDIR=lib/` and `CMAKE_POSITION_INDEPENDENT_CODE=ON` globally in depends, rather than per-package. `CMAKE_INSTALL_LIBDIR=lib/` is needed to override the annoying [`GNUInstallDirs`](https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html) `lib` vs `lib64` behaviour, and we always want PIC code. The PIC commit is the counterpart to the same Autotools change in #29488. I'm PRing these commits as I have a CMake branch building on top, and want to avoid adding the same workarounds to every package we are going to touch, but these can go in separately as the build should be tested for existing packages (i.e multiprocess).

ACKs for top commit:
  hebasto:
    re-ACK 76045bb9d6.
  theuni:
    utACK 76045bb9d6. Both changes make sense to me, and both can be overridden if needed, though I can't imagine we'd need to.

Tree-SHA512: 655a0b6b7ee5a5820f52e8e919ef03fc216d29f13f3904f72b64ce57436510e073c903039488d5740535c56e1f6221267229238c5231de5f8467d238fd562578
2024-03-25 16:47:40 +00:00
fanquake 220487bafd
Merge bitcoin/bitcoin#29718: Correct '-dbcache' to '-prune'
416b9d9427 correct '-dbcache' to '-prune' (Ben Westgate)

Pull request description:

  This looks like a typo, '-prune' is doubled not the '-dbcache' by my understanding.

ACKs for top commit:
  Sjors:
    ACK 416b9d9427
  fjahr:
    ACK 416b9d9427

Tree-SHA512: d954a8976f6e43e0af9bc2ba89fec1a1ccc90483cc005af3d2810379bd6bea7dbd87766af71c2a45b3d751a3f682f0f67a4525ec350bf6163bc4bb88fab6e4cb
2024-03-25 16:21:20 +00:00
Greg Sanders 7b29119d79 use const ref for client_maxfeerate 2024-03-25 11:52:12 -04:00
Greg Sanders f10fd07320 scripted-diff: Rename max_sane_feerate to client_maxfeerate
-BEGIN VERIFY SCRIPT-
git grep -l 'max_sane_feerate' | xargs sed -i 's/max_sane_feerate/client_maxfeerate/g'
-END VERIFY SCRIPT-
2024-03-25 11:48:18 -04:00
fanquake 2e1c84b333
Merge bitcoin/bitcoin#29660: lint: Fix COMMIT_RANGE issues
fa1146d01b lint: Fix COMMIT_RANGE issues (MarcoFalke)

Pull request description:

  `COMMIT_RANGE` has problems on forks or local branches:

  * When `LOCAL_BRANCH` is set, it assumes the presence of a `master` branch, and that the `master` branch is up-to-date. Both of which may be false. (See also discussion in https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1504226422)
  * When `COMMIT_RANGE` isn't set in `lint-git-commit-check.py`, and `--prev-commits` isn't set either, it has the same (broken) assumptions.

  Fix all issues by simply assuming a merge commit exists. This allows to drop `LOCAL_BRANCH`. It also allows to drop `SKIP_EMPTY_NOT_A_PR`, because scripts will already skip an empty range. Finally, it allows to drop `--prev-commits n`, because one can simply say `COMMIT_RANGE='HEAD~n..HEAD'` to achieve the same.

ACKs for top commit:
  Sjors:
    tACK fa1146d01b

Tree-SHA512: f1477a38267fd4fdb8d396211a5d6bed5f418798c7edaba43487957aaf726cd45244ccf15187b3dd896d398fa1df3fe0a37323e49cf44d60a2018786ed41e5ba
2024-03-25 14:41:05 +00:00
fanquake 53f4607cc8
Merge bitcoin/bitcoin#29696: tidy: remove todo, set minimum CMake to 3.22
11ee058ef5 tidy: remove C compiler check (fanquake)
c3a4ea1971 tidy: set CMAKE_CXX_STANDARD to 20 (fanquake)
5b690aeb15 tidy: remove terminfo TODO (fanquake)
24410e560a tidy: set minimum CMake to 3.22 (fanquake)

Pull request description:

  See https://github.com/hebasto/bitcoin/pull/123 for the minimum version bump.

ACKs for top commit:
  hebasto:
    re-ACK 11ee058ef5.

Tree-SHA512: 94a508ea24bf17919961bbdbc2e9d17658858e179c3b2017d5932557af32530d9d6aab197453aa5444f5478c417129c5a8e39522ff82bafac0d5a6966c7246d3
2024-03-25 12:55:52 +00:00
fanquake 76045bb9d6
depends: always set CMAKE_POSITION_INDEPENDENT_CODE=ON
Rather than potentially having to set this per-package, set it globally,
as this should always be what we want. Without doing this, changes in
later commits will have to add this per-package.

Similar to https://github.com/bitcoin/bitcoin/pull/29488, which is the
Autotools equivalent.
2024-03-25 10:51:45 +00:00
fanquake d04623678c
depends: always set CMAKE_INSTALL_LIBDIR=lib/
Rather than setting this per package, set it globally, as this is always
what we want. Without doing this, later commit will have to add the same
doc + change to more packages.
2024-03-25 10:51:08 +00:00
fanquake 556074143f
Merge bitcoin/bitcoin#29488: depends: always configure with `--with-pic`
e037c4fe09 depends: always configure with --with-pic (fanquake)

Pull request description:

  We currently do this sporadically. Not only amongst packages, but across OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

  Configure with `--with-pic` globally instead. I think this generally makes more sense, and should not have any downsides.

  See related discussion in https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1399123100.

ACKs for top commit:
  hebasto:
    ACK e037c4fe09.

Tree-SHA512: efc743ff92f9f99f3ac16514e98363ad395c6f956cd4be7e785b5c573685baf7fcd68c51d6a705ee8761fc676eb045b7e61676595be0eb0f70f34e99174cddc0
2024-03-25 10:50:25 +00:00
fanquake f22bca6473
Merge bitcoin/bitcoin#29636: test: #29007 follow ups
9a44a20fb7 init: clarify -test error (0xb10c)
3047c3e3a9 addrman: drop /*deterministic=*/ comment (0xb10c)
89b84ea91a test: check that addrman seeding is successful (0xb10c)

Pull request description:

  A few, small follow-ups to #29007. See commit messages for details.

ACKs for top commit:
  maflcko:
    lgtm ACK 9a44a20fb7
  stratospher:
    tested ACK 9a44a20.
  mzumsande:
    Code Review ACK 9a44a20fb7

Tree-SHA512: 987245e035da08fa7fe541a1dc3b7c2d90f703a6f9813875048d286335c63ffa5201db702a3f368087c00fa02c3fdafb06cf54dc7a92922749a94588b1500e98
2024-03-25 10:48:05 +00:00
Ben Westgate 416b9d9427
correct '-dbcache' to '-prune' 2024-03-25 01:39:22 -05:00
0xb10c 9a44a20fb7
init: clarify -test error
See https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1469388717
2024-03-23 15:33:50 +01:00
0xb10c 3047c3e3a9
addrman: drop /*deterministic=*/ comment
Just having deterministic is enough. See https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1488241966
2024-03-23 15:33:47 +01:00
0xb10c 89b84ea91a
test: check that addrman seeding is successful
The addpeeraddress calls can fail due to collisions. As we are using a
deteministic addrman, they won't fail with the current bucket/position
calculation. However, if the calculation is changed, they might collide
and fail silently causing tests using `seed_addrman()` to fail.

Assert that the addpeeraddress calls are successful.
2024-03-23 15:33:38 +01:00
Ava Chow c1223188e0
Merge bitcoin/bitcoin#29672: validation: Make translations of fatal errors consistent
824f47294a node: Use log levels in noui_ThreadSafeMessageBox (TheCharlatan)
ddc7872c08 node: Make translations of fatal errors consistent (TheCharlatan)

Pull request description:

  The extra `bilingual_str` argument of the fatal error notifications and `node::AbortNode()` is often unused and when used usually contains the same string as the message argument. It also seems to be confusing, since it is not consistently used for errors requiring user action. For example some assumeutxo fatal errors require the user to do something, but are not translated.

  So simplify the fatal error and abort node interfaces by only passing a translated string. This slightly changes the fatal errors displayed to the user.

ACKs for top commit:
  stickies-v:
    re-ACK 824f47294a
  maflcko:
    ACK 824f47294a 🔎
  achow101:
    ACK 824f47294a
  hebasto:
    re-ACK 824f47294a.

Tree-SHA512: 2868ee7b045fe7f3ac582ce5039141b398480b7627734976201dafaaef7544b8461635a7292fee4a7f32ff1bfc26f9bd4d0c292dca424ba42fb7fc4483d7ce8d
2024-03-22 14:50:58 -04:00
Ava Chow 2795e89cc5
Merge bitcoin/bitcoin#28998: rpc: "addpeeraddress tried" return error on failure
99954f914f test: fix test to ensure hidden RPC is present in detailed help (stratospher)
0d01f6f0c6 test: remove unused mocktime in test_addpeeraddress (0xb10c)
6205466512 rpc: "addpeeraddress tried" return error on failure (0xb10c)

Pull request description:

  When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue #28964.

  This is fixed by new returning `{ "success":  false, "error": "..." }` for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. This is done in the functional test for the `getrawaddrman` RPC.

  Fixes #28964

ACKs for top commit:
  achow101:
    ACK 99954f914f
  stratospher:
    reACK 99954f9. 🚀
  brunoerg:
    utACK 99954f914f

Tree-SHA512: 2f1299410c0582ebc2071271ba789a8abed905f9a510821f77afbcf2a555ec31397578ea55cbcd162fb828be27afedd3246c7b13ad8883f2f745bb8e04364a76
2024-03-22 14:14:30 -04:00
fanquake a175efe768
Merge bitcoin/bitcoin#29704: test: make p2p_handshake robust against timeoffset warnings
032a597482 test: make p2p_handshake robust against timeoffset warnings (stickies-v)

Pull request description:

  The new `p2p_handshake` test requires that limited nodes are not peered with when the node's system time exceeds ~ 24h of the node's chaintip timestamp, as per [`PeerManagerImpl::GetDesirableServiceFlags`](2ffaa92702/src/net_processing.cpp (L1717)).

  By patching this test to modify the timestamp of the chaintip as opposed to mocking the node's system time, we make it resilient to future commits where the node raises a warning if it detects its system time is too much out of sync with its outbound peers.

  Resolves a silent merge conflict in https://github.com/bitcoin/bitcoin/pull/29623, that is changing the warning behaviour when significant time differences with outbound peers are detected, [failing the test as it's currently in master](https://cirrus-ci.com/task/6553996884705280?logs=ci#L4666).

  Considerations/alternatives I've thought of:
  - could add `self.setup_clean_chain = True` to `self.set_test_params()`, to avoid creating a new tip with a (much) older date, but it doesn't seem to matter?
  - could avoid using `setmocktime` altogether and instead use [`create_block`](2ffaa92702/test/functional/test_framework/blocktools.py (L68)) instead, but that seems like it'll be a lot more verbose and I don't think it's worth it?

  Big thanks to theStack for his time in discussing this with me offline.

ACKs for top commit:
  maflcko:
    lgtm ACK 032a597482
  theStack:
    ACK 032a597482
  brunoerg:
    crACK 032a597482
  BrandonOdiwuor:
    Code Review ACK 032a597482

Tree-SHA512: 407564754a100bc9252f5737182de2e111993944ec9a0463a4a43195ce98cd1119de982c8fe5f7531ddb56603043812bf7bf2163a780d30b6df03a072c3308c3
2024-03-22 17:53:52 +00:00
Ava Chow 85c8a5ec48
Merge bitcoin/bitcoin#29647: Avoid divide-by-zero in header sync logs when NodeClock is behind
fa4d98b3c8 Avoid divide-by-zero in header sync logs when NodeClock is behind (MarcoFalke)
fa58550317 refactor: Modernize header sync logs (MarcoFalke)

Pull request description:

  The log may be confusing, when the NodeClock is behind the current header tip.

  Fix it, by assuming the NodeClock is never behind the current header tip.

ACKs for top commit:
  sipa:
    utACK fa4d98b3c8
  sr-gi:
    tACK [fa4d98b](fa4d98b3c8)
  achow101:
    ACK fa4d98b3c8
  tdb3:
    ACK fa4d98b3c8

Tree-SHA512: 3c5aee4030af387695918c5238012c972ebf850b52e956b5f74590cd7fd4eff0b3e593d411e3eb2a0bb12294af8dc6fbe320f90e4c261399b65a404ff3c3cbd9
2024-03-22 13:18:30 -04:00
stickies-v 032a597482
test: make p2p_handshake robust against timeoffset warnings
The test requires that limited nodes are not peered with  when
the node's system time exceeds ~ 24h of the node's chaintip
timestamp, as per PeerManagerImpl::GetDesirableServiceFlags.

By patching this test to modify the timestamp of the chaintip as
opposed to mocking the node's system time, we make it resilient
to future commits where the node raises a warning if it detects
its system time is too much out of sync with its outbound peers.

See https://github.com/bitcoin/bitcoin/pull/29623
2024-03-22 14:36:52 +00:00
fanquake 11ee058ef5
tidy: remove C compiler check
Also requires disabling FFI.
2024-03-22 13:48:00 +00:00
fanquake c3a4ea1971
tidy: set CMAKE_CXX_STANDARD to 20 2024-03-22 13:38:23 +00:00
fanquake 5b690aeb15
tidy: remove terminfo TODO
At the same time, also disable searching for CURL, LibEdit, LibXml2,
ZLIB and zstd none of which we use.
2024-03-22 13:33:10 +00:00
fanquake 24410e560a
tidy: set minimum CMake to 3.22
Matches https://github.com/hebasto/bitcoin/pull/123.
This also also dev/ci only code.
2024-03-22 12:32:36 +00:00
fanquake 2ffaa92702
Merge bitcoin/bitcoin#29703: doc: Rename `contrib/devtools/bitcoin-tidy/README` to `README.md`
669ea0aa4a doc: Rename `contrib/devtools/bitcoin-tidy/README` to `README.md` (Hennadii Stepanov)

Pull request description:

  This PR fixes the file formatting on the GitHub website.

  Before:
  ![image](https://github.com/bitcoin/bitcoin/assets/32963518/e81a61f0-f18c-4917-ae79-d35807e91fa9)

  After:
  ![image](https://github.com/bitcoin/bitcoin/assets/32963518/d2a96317-06d6-4185-b0e8-5e62c75f66b5)

ACKs for top commit:
  maflcko:
    lgtm ACK 669ea0aa4a

Tree-SHA512: 3dfa07a482b5447b6f423946cc463648abbedf1012f2da246b8ce16e6ae3895cdbf956fb2db78735f3e0d88473b9b32f226f2557e0004a0adb69b00f2426a8ca
2024-03-22 11:24:11 +00:00
Hennadii Stepanov 669ea0aa4a
doc: Rename `contrib/devtools/bitcoin-tidy/README` to `README.md`
This change fixes the file formatting on the GitHub website.
2024-03-22 10:53:21 +00:00
MarcoFalke fa1146d01b
lint: Fix COMMIT_RANGE issues 2024-03-21 20:15:08 +01:00
fanquake 7850c5fe20
guix: build GCC with --enable-standard-branch-protection
To enable Branch Target Identification Mechanism and Return
Address Signing by default at configure time use the
`--enable-standard-branch-protection` option.

This is equivalent to having `-mbranch-protection=standard` during
compilation. This can be explicitly disabled during compilation
by passing the `-mbranch-protection=none` option which turns off
all types of branch protections.

See:
https://gcc.gnu.org/install/specific.html#aarch64-x-x
2024-03-21 17:33:59 +00:00
TheCharlatan 824f47294a
node: Use log levels in noui_ThreadSafeMessageBox 2024-03-21 16:41:16 +01:00
TheCharlatan ddc7872c08
node: Make translations of fatal errors consistent
The extra `bilingual_str` argument of the fatal error notifications and
`node::AbortNode()` is often unused and when used usually contains the
same string as the message argument. It also seems to be confusing,
since it is not consistently used for errors requiring user action. For
example some assumeutxo fatal errors require the user to do something,
but are not translated.

So simplify the fatal error and abort node interfaces by only passing a
translated string. This slightly changes the fatal errors displayed to
the user.

Also de-duplicate the abort error log since it is repeated in noui.cpp.
2024-03-21 16:40:22 +01:00
fanquake 71b63195b3
Merge bitcoin/bitcoin#29651: guix: bump time-machine to dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a
cf5faf73c9 guix: bump time-machine to dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a (fanquake)

Pull request description:

  This includes a commit to fix building LLVM 17 on riscv64, see https://git.savannah.gnu.org/cgit/guix.git/commit/?id=4e26331a5ee87928a16888c36d51e270f0f10f90.

  Followup to discussion in https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1843313196.

  If you don't have riscv64 hardware, this can be tested with the following:
  ```bash
  # observe failure when cross-compiling using our current time-machine
  guix time-machine --commit=d5ca4d4fd713a9f7e17e074a1e37dda99bbb09fc -- build --target=riscv64-linux-gnu  llvm
  ....
  riscv64-linux-gnu-ld: CMakeFiles/dsymutil.dir/dsymutil.cpp.o: undefined reference to symbol '__atomic_fetch_and_1@@LIBATOMIC_1.0'
  riscv64-linux-gnu-ld: /gnu/store/i4ga0pnr1b74bir2bjyp8mcrrbsvk7d3-gcc-cross-riscv64-linux-gnu-11.3.0-lib/riscv64-linux-gnu/lib/libatomic.so.1:
    error adding symbols: DSO missing from command line
  collect2: error: ld returned 1 exit status

  # build success when using the new time-machine
  guix time-machine --commit=dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a -- build --target=riscv64-linux-gnu  llvm
  ....
  grafting '/gnu/store/7y0j0y8jaz4mjx2nz0y42wdnxxjp6id6-llvm-17.0.6-opt-viewer' -> '/gnu/store/8xvahrrjscbprh6cjj0qp5bm9mm78wwa-llvm-17.0.6-opt-viewer'...
  grafting '/gnu/store/bjhw648bz7ijd2p9hgzzdbw1q8hpagk8-llvm-17.0.6' -> '/gnu/store/x50qi8i2ywgpx6azv4k55ms0w5xjxxg5-llvm-17.0.6'...
  successfully built /gnu/store/q9xvk8gzzvb4dxfzf6yi5164zd0d1vj2-llvm-17.0.6.drv
  ```

  Also includes at least:
  Linux Headers 6.1.67 -> 6.1.80

ACKs for top commit:
  TheCharlatan:
    ACK cf5faf73c9
  hebasto:
    ACK cf5faf73c9, tested on x86_64 hardware as described in the PR description.

Tree-SHA512: b49d4f90effeec666b12b5447a24c90315b82675cfc166bc1230ac173134bab6b277fc7e064bbb75e990275165b2b27d88e4ec1cdeea4750541ec6443cb50f41
2024-03-21 11:03:25 +00:00
Ava Chow bf1b6383db
Merge bitcoin/bitcoin#29671: index: avoid "failed to commit" errors on initialization
f65b0f6401 index: Move last_locator_write_time and logging to end of threadsync loop (Fabian Jahr)

Pull request description:

  In the index sync thread, when initializing an index for the first time, stop callng BaseIndex::Commit when m_best_block_index is null, to avoid a spurious "failed to commit" error from that function. This error started happening in commit 7878f97bf1 from https://github.com/bitcoin/bitcoin/pull/25494 and was reported by pstratem in https://github.com/bitcoin/bitcoin/pull/26903 with an alternate fix.

ACKs for top commit:
  achow101:
    ACK f65b0f6401
  ryanofsky:
    Code review ACK f65b0f6401. Just moved log "Syncing" log line since last commit to avoid having to call now() twice.
  furszy:
    ACK f65b0f6401
  TheCharlatan:
    ACK f65b0f6401

Tree-SHA512: afa8f05786318d36346d167ff53ea0b3bc8abdb0ad04465d199dc3eb91e9f837369e24fcb7e24b5757b02d698ec504e61da6ac365eaf006c874fc07a424a7e20
2024-03-20 16:38:32 -04:00
ishaanam 5952292133 wallet, rpc: show mempool conflicts in `gettransaction` result 2024-03-20 15:05:37 -04:00
ishaanam 54e07ee22f wallet: track mempool conflicts
Behavior changes are:
- if a tx has a mempool conflict, the wallet will not attempt to
  rebroadcast it
- if a txo is spent by a mempool-conflicted tx, that txo is no
  longer considered spent
2024-03-20 15:05:34 -04:00
Ava Chow b50554babd
Merge bitcoin/bitcoin#29370: assumeutxo: Get rid of faked nTx and nChainTx values
9d9a7458a2 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9ed2 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458d76 test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b662 assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5bbf9 doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915ee6b validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc912c ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e687ec assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)

Pull request description:

  The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.

  Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.

  Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.

  Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.

ACKs for top commit:
  Sjors:
    re-ACK 9d9a7458a2
  achow101:
    ACK 9d9a7458a2
  mzumsande:
    Tested ACK 9d9a7458a2
  maflcko:
    ACK 9d9a7458a2 🎯

Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
2024-03-20 12:56:49 -04:00
Ava Chow 69ddee6f39
Merge bitcoin/bitcoin#27039: blockstorage: do not flush block to disk if it is already there
dfcef536d0 blockstorage: do not flush block to disk if it is already there (Matthew Zipkin)

Pull request description:

  Closes https://github.com/bitcoin/bitcoin/issues/2039

  When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances.

  `FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time.

  The call stack looks like this:

  ```
  init()
  ThreadImport() <-- process fReindex flag
  LoadExternalBlockFile()
  AcceptBlock()
  SaveBlockToDisk()
  FindBlockPos()
  FlushBlockFile() <-- unnecessary if block is already on disk
  ```

  A larger refactor of this part of the code was started by mzumsande here:  https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined.

ACKs for top commit:
  sipa:
    utACK dfcef536d0
  mzumsande:
    re-ACK dfcef536d0
  achow101:
    ACK dfcef536d0
  furszy:
    Rebase diff ACK dfcef53.

Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279
2024-03-20 12:41:33 -04:00
Ava Chow 0b96a1925e
Merge bitcoin/bitcoin#28955: index: block filters sync, reduce disk read operations by caching last header
99afb9d15a refactor: init, simplify index shutdown code (furszy)
0faafb57f8 index: decrease ThreadSync cs_main contention (furszy)
f1469eb454 index: cache last block filter header (furszy)
a6756ecdb2 index: blockfilter, decouple header lookup into its own function (furszy)
331f044e3b index: blockfilter, decouple Write into its own function (furszy)
bcbd7eb8d4 bench: basic block filter index initial sync (furszy)

Pull request description:

  Work decoupled from #26966 per request.

  The aim is to remove an unnecessary disk read operation that currently takes place with every new arriving block (or scanned block during background sync). Instead of reading the last filter header from disk merely to access its hash for constructing the next filter, this work caches it, occupying just 32 more bytes in memory.

  Also, reduces `cs_main` lock contention during the index initial sync process. And, simplifies the indexes initialization and shutdown procedure.

  Testing Note:
  To compare the changes, added a pretty basic benchmark in the second commit. Alternatively, could also test the changes by timing the block filter sync from scratch on any network; start the node with `-blockfilterindex` and monitor the logs until the syncing process finish.

  Local Benchmark Results:

  *Master (c252a0fc0f):
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |      132,042,516.60 |                7.57 |    0.3% |      7.79 | `BlockFilterIndexSync`

  *PR (43a212cfdac6c64e82b601c664443d022f191520):
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |      126,915,841.60 |                7.88 |    0.6% |      7.51 | `BlockFilterIndexSync`

ACKs for top commit:
  Sjors:
    re-ACK 99afb9d15a
  achow101:
    ACK 99afb9d15a
  TheCharlatan:
    Re-ACK 99afb9d15a
  andrewtoth:
    ACK 99afb9d15a

Tree-SHA512: 927daadd68f4ee1ca781a89519539b895f5185a76ebaf525fbc246ea8dcf40d44a82def00ac34b188640802844b312270067f1b33e65a2479e06be9169c616de
2024-03-20 12:30:38 -04:00