0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-20 12:12:41 -05:00
Commit graph

5223 commits

Author SHA1 Message Date
Greg Sanders
a0376e1061 unit test: clarify unstated assumption for calc_feerate_diagram_rbf chunking 2024-03-26 08:20:30 -04:00
Greg Sanders
890cb015f3 s/effected/affected/ 2024-03-26 08:20:30 -04:00
Greg Sanders
d9391ec095 CalculateFeerateDiagramsForRBF: remove size tie-breaking from chunking conflicts 2024-03-26 08:20:30 -04:00
Greg Sanders
b684d82d7e fuzz: Add more invariant checks for package_rbf 2024-03-26 08:20:30 -04:00
Greg Sanders
2a3ada8b21 fuzz: finer grained ImprovesFeerateDiagram check on error result 2024-03-26 08:20:30 -04:00
Greg Sanders
c377ae9ba0 unit test: improve ImprovesFeerateDiagram coverage with one less vb case 2024-03-26 08:20:30 -04:00
Greg Sanders
d2bf923eb1 unit test: make calc_feerate_diagram_rbf less brittle 2024-03-26 08:20:30 -04:00
Greg Sanders
defe023f6e fuzz: add PrioritiseTransaction coverage in diagram checks 2024-03-26 08:20:30 -04:00
Greg Sanders
216d5ff162 unit test: add coverage showing priority affects diagram check results 2024-03-26 08:20:30 -04:00
Greg Sanders
a80d80936a unit test: add CheckConflictTopology case for not the only child 2024-03-26 08:20:30 -04:00
Greg Sanders
69bd18ca80 unit test: check tx4 conflict error message 2024-03-26 08:05:22 -04:00
Greg Sanders
c0c37f07eb unit test: have CompareFeerateDiagram tested with diagrams both ways 2024-03-26 08:05:22 -04: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
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
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
fanquake
479ecc0515
Merge bitcoin/bitcoin#29192: Weaken serfloat tests
6e873df347 serfloat: improve/simplify tests (Pieter Wuille)
b45f1f5658 serfloat: do not test encode(bits)=bits anymore (Pieter Wuille)

Pull request description:

  Closes #28941.

  Our current tests for serfloat verify two distinct properties:
  1. Whether they roundtrip `double`->`uint64_t`->`double` (excluding NaN values) on all systems.
  2. Whether on systems with a typical floating point unit that encoding matches the hardware representation, as before v22.0, we would dump the hardware representation directly to disk and we wanted to retain compatibility with that.

  #28941 seems to show that the second property doesn't always hold, but just for "subnormal" numbers (below $2^{-1021}$). Since we don't care about encoding these numbers, we could exclude such subnormal numbers from the hardware-identical representation test, but this PR goes further and just drops the second property entirely, as I don't think we care about edge-case compatibility with pre-v22.0 code for fee_estimates.dat (the only place it is used).

ACKs for top commit:
  glozow:
    ACK 6e873df347
  fanquake:
    ACK 6e873df347 - It's not as much of a priority, but I think we could still backport this.

Tree-SHA512: e18ceee0753a7ee7e999fdfa10b014dc5bb67b6ef79522a0f8c76b889adcfa785772fc26ed7559bcb5a09a9938e243bb54eedd9549bc59080a2c8090155e2267
2024-03-19 17:09:07 +00:00
fanquake
0f89e86516
Merge bitcoin/bitcoin#29667: fuzz: actually test garbage >64b in p2p transport test
626f8e398e fuzz: actually test garbage >64b in p2p transport test (Pieter Wuille)

Pull request description:

  This fixes an oversight from #28196: in the `p2p_transport_bidirectional_v2` fuzz test, when the desired garbage length is over 64 bytes, the code would actually use garbage length 0. Fix this.

ACKs for top commit:
  instagibbs:
    ACK 626f8e398e
  brunoerg:
    crACK 626f8e398e

Tree-SHA512: f6346367adb10464b6c9d20aef43625531d2a4d8110887ad03214b8c1907b83560f2dd5b5415e2180a40b4cd276d51881b32b60c740471b5c6bb218aa19848d8
2024-03-19 12:20:33 +00:00
glozow
5d045c31a5
Merge bitcoin/bitcoin#28950: RPC: Add maxfeerate and maxburnamount args to submitpackage
38f70ba6ac RPC: Add maxfeerate and maxburnamount args to submitpackage (Greg Sanders)

Pull request description:

  Resolves https://github.com/bitcoin/bitcoin/issues/28949

  I couldn't manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from https://github.com/bitcoin/bitcoin/pull/19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.

  The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.

ACKs for top commit:
  ismaelsadeeq:
    Re-ACK 38f70ba6ac 👍🏾
  glozow:
    ACK 38f70ba6ac with some non-blocking nits
  murchandamus:
    LGTM, code review ACK 38f70ba6ac

Tree-SHA512: 38212aa9de25730944cee58b0806a3d37097e42719af8dd7de91ce86bb5d9770b6f7c37354bf418bd8ba571c52947da1dcdbb968bf429dd1dbdf8715315af18f
2024-03-18 18:24:06 +00:00
fanquake
80f8b92f4f
remove libbitcoinconsensus
This was deprecated in v27.0, for removal in v28.0.
See discussion in PR #29189.
2024-03-18 16:59:39 +00:00
Ryan Ofsky
9d9a7458a2 assumeutxo: Remove BLOCK_ASSUMED_VALID flag
Flag adds complexity and is not currently used for anything.
2024-03-18 11:28:40 -05:00
Ryan Ofsky
ef29c8b662 assumeutxo: Get rid of faked nTx and nChainTx values
The `PopulateAndValidateSnapshot` function introduced in
f6e2da5fb7 from #19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see #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.

This commit fixes at least two assert failures in the (pindex->nChainTx ==
pindex->nTx + prev_chain_tx) check that would happen previously. Tests for
these failures are added separately in the next two commits.

Compatibility note: This change could result in -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.
2024-03-18 11:28:40 -05:00
MarcoFalke
fa72dcbfa5
refactor: FormatISO8601* without gmtime* 2024-03-18 16:01:24 +01:00
MarcoFalke
fa2c486afc
Revert "time: add runtime sanity check"
This reverts commit 3c2e16be22.
2024-03-18 16:01:08 +01:00
Greg Sanders
7295986778 Unit tests for CalculateFeerateDiagramsForRBF 2024-03-18 10:32:00 -04:00
Greg Sanders
b767e6bd47 test: unit test for ImprovesFeerateDiagram 2024-03-18 10:32:00 -04:00
Greg Sanders
7e89b659e1 Add fuzz test for FeeFrac 2024-03-18 10:32:00 -04:00
Greg Sanders
4d6528a3d6 fuzz: fuzz diagram creation and comparison
Co-authored-by: Suhas Daftuar <sdaftuar@chaincode.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
2024-03-18 10:32:00 -04:00
Greg Sanders
e9c5aeb11d test: Add tests for CompareFeerateDiagram and CheckConflictTopology 2024-03-18 10:32:00 -04:00
Greg Sanders
588a98dccc fuzz: Add fuzz target for ImprovesFeerateDiagram
Co-authored-by: Suhas Daftuar <sdaftuar@chaincode.com>
2024-03-18 10:32:00 -04:00
Greg Sanders
66d966dcfa Add FeeFrac unit tests
Co-authored-by: Suhas Daftuar <sdaftuar@chaincode.com>
2024-03-18 10:32:00 -04:00
Pieter Wuille
626f8e398e fuzz: actually test garbage >64b in p2p transport test 2024-03-17 11:35:01 -04:00
Ava Chow
ef6329f052
Merge bitcoin/bitcoin#28193: test: add script compression coverage for not-on-curve P2PK outputs
28287cfbe1 test: add script compression coverage for not-on-curve P2PK outputs (Sebastian Falbesoner)

Pull request description:

  This PR adds unit test coverage for the script compression functions `{Compress,Decompress}Script` in the special case of uncompressed P2PK outputs (scriptPubKey: OP_PUSH65 <0x04 ....> OP_CHECKSIG) with [pubkeys that are not fully valid](44b05bf3fe/src/pubkey.cpp (L297-L302)), i.e. where the encoded point is not on the secp256k1 curve. For those outputs, script compression is not possible, as the y coordinate of the pubkey can't be recovered (see also call-site of `IsToPubKey`):

  44b05bf3fe/src/compressor.cpp (L49-L50)

  Likewise, for a compressed script of an uncompressed P2PK script (i.e. compression ids 4 and 5) where the x coordinate is not on the curve, decompression fails:

  44b05bf3fe/src/compressor.cpp (L122-L129)

  Note that the term "compression" is used here in two different meanings (though they are related), which might be a little confusing. The encoding of a pubkey can either be compressed (33-bytes with 0x02/0x03 prefixes) or uncompressed (65-bytes with 0x04 prefix). On the other hand there is also compression for whole output scripts, which is used for storing scriptPubKeys in the UTXO set in a compact way (and also for the `dumptxoutset` result, accordingly). P2PK output scripts with uncompressed pubkeys get compressed by storing only the x-coordinate and the sign as a prefix (0x04 = even, 0x05 = odd). Was diving deeper into the subject while working on https://github.com/bitcoin/bitcoin/pull/27432, where the script decompression of uncompressed P2PK needed special handling (see also https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-1108798536).

  Trivia: as of now (block 801066), there are 13 uncompressed P2PK outputs in the UTXO set with a pubkey not on the curve (which obviously means they are unspendable).

ACKs for top commit:
  achow101:
    ACK 28287cfbe1
  tdb3:
    ACK for 28287cfbe1.
  cbergqvist:
    ACK 28287cf!
  marcofleon:
    Nicely done, ACK 28287cfbe1. Built the PR branch, ran the unit and functional tests, everything passed.

Tree-SHA512: 777b6c3065654fbfa1ce94926f4cadb91a9ca9dc4dd4af6008ad77bd1da5416f156ad0dfa880d26faab2e168bf9b27e0a068abc9a2be2534d82bee61ee055c65
2024-03-13 11:02:23 -04:00
Greg Sanders
38f70ba6ac RPC: Add maxfeerate and maxburnamount args to submitpackage
And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.
2024-03-13 09:45:43 -04:00
Ava Chow
0ed2c130e7
Merge bitcoin/bitcoin#27375: net: support unix domain sockets for -proxy and -onion
567cec9a05 doc: add release notes and help text for unix sockets (Matthew Zipkin)
bfe5192891 test: cover UNIX sockets in feature_proxy.py (Matthew Zipkin)
c65c0d0163 init: allow UNIX socket path for -proxy and -onion (Matthew Zipkin)
c3bd43142e gui: accomodate unix socket Proxy in updateDefaultProxyNets() (Matthew Zipkin)
a88bf9dedd i2p: construct Session with Proxy instead of CService (Matthew Zipkin)
d9318a37ec net: split ConnectToSocket() from ConnectDirectly() for unix sockets (Matthew Zipkin)
ac2ecf3182 proxy: rename randomize_credentials to m_randomize_credentials (Matthew Zipkin)
a89c3f59dc netbase: extend Proxy class to wrap UNIX socket as well as TCP (Matthew Zipkin)
3a7d6548ef net: move CreateSock() calls from ConnectNode() to netbase methods (Matthew Zipkin)
74f568cb6f netbase: allow CreateSock() to create UNIX sockets if supported (Matthew Zipkin)
bae86c8d31 netbase: refactor CreateSock() to accept sa_family_t (Matthew Zipkin)
adb3a3e51d configure: test for unix domain sockets (Matthew Zipkin)

Pull request description:

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

  UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.

  There has been work on [unix domain sockets before](https://github.com/bitcoin/bitcoin/pull/9979) but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by `-onion=` or `-proxy=`

  I copied the prefix `unix:` usage from Tor. With this patch built locally you can test with your own filesystem path (example):

  `tor --SocksPort unix:/Users/matthewzipkin/torsocket/x`

  `bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x`

  Prep work for this feature includes:
  - Moving where and how we create `sockaddr` and `Sock` to accommodate `AF_UNIX` without disturbing `CService`
  - Expanding `Proxy` class to represent either a `CService` or a UNIX socket (by its file path)

  Future work:
  - Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
  - Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
  - Enable UNIX sockets on windows where supported
  - Update Network Proxies dialog in GUI to support UNIX sockets

ACKs for top commit:
  Sjors:
    re-ACK 567cec9a05
  tdb3:
    re ACK for 567cec9a05.
  achow101:
    ACK 567cec9a05
  vasild:
    ACK 567cec9a05

Tree-SHA512: de81860e56d5de83217a18df4c35297732b4ad491e293a0153d2d02a0bde1d022700a1131279b187ef219651487537354b9d06d10fde56225500c7e257df92c1
2024-03-13 06:53:07 -04:00
Ava Chow
bef99176e6
Merge bitcoin/bitcoin#27114: p2p: Allow whitelisting manual connections
0a533613fb docs: add release notes for #27114 (brunoerg)
e6b8f19de9 test: add coverage for whitelisting manual connections (brunoerg)
c985eb854c test: add option to speed up tx relay/mempool sync (brunoerg)
66bc6e2d17 Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections (Luke Dashjr)
8e06be347c net_processing: Move extra service flag into InitializeNode (Luke Dashjr)
9133fd69a5 net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` (Luke Dashjr)
2863d7dddb net: store `-whitelist{force}relay` values in `CConnman` (brunoerg)

Pull request description:

  Revives #17167. It allows whitelisting manual connections. Fixes #9923

  Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:
  - Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology `node0 <--- node1 <---- node2 <--- ... <-- nodeN`,  if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
  - https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764 - "Before enabling -v2transport by default (which I'd image may happen after https://github.com/bitcoin/bitcoin/pull/24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if https://github.com/bitcoin/bitcoin/pull/27114 makes it in."
  - https://github.com/bitcoin/bitcoin/pull/17167#issuecomment-1168606032 - "This would allow us to use https://github.com/bitcoin/bitcoin/pull/25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
  - Force-relay/mempool permissions for a node you intentionally connected to.

ACKs for top commit:
  achow101:
    ACK 0a533613fb
  sr-gi:
    re-ACK [0a53361](0a533613fb)
  pinheadmz:
    ACK 0a533613fb

Tree-SHA512: 97a79bb854110da04540897d2619eda409d829016aafdf1825ab5515334b0b42ef82f33cd41587af235b3af6ddcec3f2905ca038b5ab22e4c8a03d34f27aebe1
2024-03-12 12:59:02 -04:00
Ava Chow
12dae637a4
Merge bitcoin/bitcoin#29306: policy: enable sibling eviction for v3 transactions
1342a31f3a [functional test] sibling eviction (glozow)
5fbab37859 [unit test] sibling not returned from SingleV3Checks if 1p2c or 3gen (glozow)
170306728a [policy] sibling eviction for v3 transactions (glozow)
b5d15f764f [refactor] return pair from SingleV3Checks (glozow)

Pull request description:

  When we receive a v3 transaction that would bust a mempool transaction's descendant limit, instead of rejecting the new tx, consider replacing the other descendant if it is much higher feerate (using existing RBF criteria to assess that it's more incentive compatible and to avoid DoS).

  Delving post with more background and motivation: https://delvingbitcoin.org/t/sibling-eviction-for-v3-transactions/472

ACKs for top commit:
  sdaftuar:
    ACK 1342a31f3a
  achow101:
    ACK 1342a31f3a
  instagibbs:
    ACK 1342a31f3a

Tree-SHA512: dd957d49e51db78758f566c49bddc579b72478e371275c592d3d5ba097d20de47a6c81952045021b99d82a787f5b799baf16dd0ee0e6de90ba12e21e275352be
2024-03-12 12:19:48 -04:00
Ava Chow
5ebb406357
Merge bitcoin/bitcoin#26564: test: test_bitcoin: allow -testdatadir=<datadir>
d27e2d87b9 test: test_bitcoin: allow -testdatadir=<datadir> (Larry Ruane)

Pull request description:

  This backward-compatible change would help with code review, testing, and debugging. When `test_bitcoin` runs, it creates a working or data directory within `/tmp/test_common_Bitcoin\ Core/`, named as a long random (hex) string.

  This small patch does three things:

  - If the (new) argument `-testdatadir=<datadir>` is given, use `<datadir>/test_temp/<test-name>/datadir` as the working directory
  - When the test starts, remove `<datadir>/test_temp/<test-name>/datadir` if it exists from an earlier run (currently, it's presumed not to exist due to the long random string)
  - Don't delete the working directory at the end of the test if a custom data directory is being used

  Example usage, which will remove, create, use `/somewhere/test_temp/getarg_tests/boolarg`, and leave it afterward:
  ```
  $ test_bitcoin --run_test=getarg_tests/boolarg -- -testdatadir=/somewhere
  Running 1 test case...
  Test directory (will not be deleted): "/somewhere/test_temp/getarg_tests/boolarg/datadir"

  *** No errors detected
  $ ls -l /somewhere/test_temp/getarg_tests/boolarg/datadir
  total 8
  drwxrwxr-x 2 larry larry 4096 Feb 22 10:28 blocks
  -rw-rw-r-- 1 larry larry 1273 Feb 22 10:28 debug.log
  ```
  (A relative pathname also works.)

  This change affects only `test_bitcoin`; it could also be applied to `test_bitcoin-qt` but that's slightly more involved so I'm skipping that for now.

  The rationale for this change is that, when running the test using the debugger, it's often useful to watch `debug.log` as the test runs and inspect some of the other files (I've looked at the generated `blknnnn.dat` files for example). Currently, that requires figuring out where the test's working directory is since it changes on every test run. Tests can be run with `-printtoconsole=1` to show debug logging to the terminal, but it's nice to keep `debug.log` continuously open in an editor, for example.

  Even if not using a debugger, it's sometimes helpful to see `debug.log` and other artifacts after the test completes.

  Similar functionality is already possible with the functional tests using the `--tmpdir=` and `--nocleanup` arguments.

ACKs for top commit:
  davidgumberg:
    ACK d27e2d87b9
  tdb3:
    re-ACK for d27e2d87b9
  achow101:
    ACK d27e2d87b9
  cbergqvist:
    ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a! (Already did some testing with `fs::remove()` to make sure it was compatible with the `util::Lock/UnlockDirectory` implementation).
  marcofleon:
    ACK d27e2d87b9. I ran all the tests with my previous open file limit and no errors were detected. Also ran some individual tests with no, relative, and absolute paths and everything looks good.
  furszy:
    ACK d27e2d8

Tree-SHA512: a8f535f34a48b6699cb440f97f5562ec643f3bfba4ea685768980b871fc8b6e1135f70fc05dbe19aa2c8bacb1ddeaff212d63473605a7422ff76332b3a6b1f68
2024-03-11 07:03:02 -04:00
Ava Chow
4cc99df44a
Merge bitcoin/bitcoin#29569: Rename CalculateHeadersWork to CalculateClaimedHeadersWork
eb7cc9fd21 Rename CalculateHeadersWork to CalculateClaimedHeadersWork (Greg Sanders)

Pull request description:

  And clean up some comments. Confusion about what this is doing seems to be a running theme:

  https://github.com/bitcoin/bitcoin/pull/29549#discussion_r1511113344

  https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141510303

ACKs for top commit:
  achow101:
    ACK eb7cc9fd21
  pablomartin4btc:
    ACK eb7cc9fd21
  0xB10C:
    ACK eb7cc9fd21
  dergoegge:
    ACK eb7cc9fd21
  BrandonOdiwuor:
    ACK eb7cc9fd21

Tree-SHA512: 6ccbc5e417155516487bb220753d189b5341dec05366db88a3fa5b1932eace21fbfaf23408c639bb54b36169a8d0a7536a1ee5e63b4ce5a3b70f2ff8407b6e07
2024-03-08 21:39:07 -05:00
Ava Chow
c07935bcf5
Merge bitcoin/bitcoin#28960: kernel: Remove dependency on CScheduler
d5228efb53 kernel: Remove dependency on CScheduler (TheCharlatan)
06069b3913 scripted-diff: Rename MainSignals to ValidationSignals (TheCharlatan)
0d6d2b650d scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner (TheCharlatan)
4abde2c4e3 [refactor] Make MainSignals RAII styled (TheCharlatan)
84f5c135b8 refactor: De-globalize g_signals (TheCharlatan)
473dd4b97a [refactor] Prepare for g_signals de-globalization (TheCharlatan)
3fba3d5dee [refactor] Make signals optional in mempool and chainman (TheCharlatan)

Pull request description:

  By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.

  Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.

  To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.

  Renames `CMainSignals` to `ValidationSignals`, which more accurately describes its purpose and scope.

  Also make the `ValidationSignals` in the `ChainstateManager` and CTxMemPool` optional. This could be useful in the future for using or testing these classes without having to instantiate any form of signal handling.

  ---

  This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). It improves the kernel API and removes two modules from the kernel library.

ACKs for top commit:
  maflcko:
    re-ACK d5228efb53 🌄
  ryanofsky:
    Code review ACK d5228efb53. Just comment change since last review.
  vasild:
    ACK d5228efb53
  furszy:
    diff ACK d5228ef

Tree-SHA512: e93a5f10eb6182effb84bb981859a7ce750e466efd8171045d8d9e7fe46e4065631d9f6f533c5967c4d34c9bb7d7a67e9f4593bd4c5b30cd7b3bbad7be7b331b
2024-03-08 20:58:04 -05:00
Larry Ruane
d27e2d87b9 test: test_bitcoin: allow -testdatadir=<datadir>
Specifying this argument overrides the path location for test_bitcoin;
it becomes <datadir>/test_common_Bitcoin Core/<testname>/datadir. Also,
this directory isn't removed after the test completes. This can make it
easier for developers to study the results of a test (see the state of
the data directory after the test runs), and also (for example) have an
editor open on debug.log to monitor it across multiple test runs instead
of having to re-open a different pathname each time.

Example usage (note the "--" is needed):

test_bitcoin --run_test=getarg_tests/boolarg -- \
-testdatadir=/somewhere/mydatadir

This will create (if necessary) and use the data directory:

/somewhere/mydatadir/test_common_Bitcoin Core/getarg_tests/boolarg/datadir

Co-authored-by: furszy <mfurszy@protonmail.com>
2024-03-07 10:11:45 -07:00
fanquake
312f3381a2
fuzz: restrict fopencookie usage to Linux & FreeBSD
Should fix the GCC compilation portion of #29517:
https://github.com/bitcoin/bitcoin/issues/29517#issuecomment-1973573314.

See also:
https://www.gnu.org/software/gnulib/manual/html_node/fopencookie.html
but note that FreeBSD has supported this function since 11.x.
2024-03-05 21:18:44 +00:00
Greg Sanders
eb7cc9fd21 Rename CalculateHeadersWork to CalculateClaimedHeadersWork 2024-03-05 10:01:24 -05:00
Matthew Zipkin
a88bf9dedd
i2p: construct Session with Proxy instead of CService 2024-03-01 14:47:29 -05:00
Matthew Zipkin
bae86c8d31
netbase: refactor CreateSock() to accept sa_family_t
Also implement CService::GetSAFamily() to provide sa_family_t
2024-03-01 13:13:07 -05:00
fanquake
8da62a1041
Merge bitcoin/bitcoin#29263: serialization: c++20 endian/byteswap/clz modernization
86b7f28d6c serialization: use internal endian conversion functions (Cory Fields)
432b18ca8d serialization: detect byteswap builtins without autoconf tests (Cory Fields)
297367b3bb crypto: replace CountBits with std::bit_width (Cory Fields)
52f9bba889 crypto: replace non-standard CLZ builtins with c++20's bit_width (Cory Fields)

Pull request description:

  This replaces #28674, #29036, and #29057. Now ready for testing and review.

  Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.

  I apologize for the size of the last commit, but it's hard to avoid making those changes at once.

  All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.

  Sadly, benchmarking showed that not all compilers are capable of detecting and optimizing byteswap functions, so compiler builtins are instead used where possible. However, they're now detected via macros rather than autoconf checks.

  This[ matches how libc++ implements std::byteswap for c++23](https://github.com/llvm/llvm-project/blob/main/libcxx/include/__bit/byteswap.h#L26).

  I suggest we move/rename `compat/endian.h`, but I left that out of this PR to avoid bikeshedding.

  #29057 pointed out some irregularities in benchmarks. After messing with various compilers and configs for a few weeks with these changes, I'm of the opinion that we can't win on every platform every time, so we should take the code that makes sense going forward. That said, if any real-world slowdowns are caused here, we should obviously investigate.

ACKs for top commit:
  maflcko:
    ACK 86b7f28d6c 📘
  fanquake:
    ACK 86b7f28d6c - we can finish pruning out the __builtin_clz* checks/usage once the minisketch code has been updated. This is more good cleanup pre-CMake & for the kernal.

Tree-SHA512: 715a32ec190c70505ffbce70bfe81fc7b6aa33e376b60292e801f60cf17025aabfcab4e8c53ebb2e28ffc5cf4c20b74fe3dd8548371ad772085c13aec8b7970e
2024-03-01 11:19:58 -05:00
glozow
5fbab37859 [unit test] sibling not returned from SingleV3Checks if 1p2c or 3gen 2024-03-01 15:23:04 +00:00
glozow
170306728a [policy] sibling eviction for v3 transactions 2024-03-01 15:23:03 +00:00
fanquake
ae4165f7bc
Merge bitcoin/bitcoin#29495: fuzz: add target for local address stuff
25eab52389 fuzz: add target for local addresses (brunoerg)

Pull request description:

  This PR adds fuzz target for local address functions - (`AddLocal`, `RemoveLocal`, `SeenLocal`, `IsLocal`)

ACKs for top commit:
  dergoegge:
    ACK 25eab52389
  vasild:
    ACK 25eab52389

Tree-SHA512: 24faaab86dcd8835ba0e2d81fb6322a39a9266c7edf66415dbc4421754054f47efb6e0de4efdc7ea026b0686792658e86a526f7cf27cbc6cf9ed0c4aed376f97
2024-03-01 10:07:48 -05:00
stickies-v
bbb31269bf
rpc: add named arg helper
Overload the Arg and MaybeArg helpers to allow accessing arguments
by name as well.

Also update the docs to document Arg and MaybeArg separately
2024-03-01 13:51:21 +00:00