aboutsummaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2024-06-20refactor: remove extraneous lock annotations from function definitionsCory Fields
These annotations belong in the declarations rather than the definitions. While harmless now, future versions of clang may warn about these.
2024-06-20Merge bitcoin/bitcoin#30202: netbase: extend CreateSock() to support ↵Ava Chow
creating arbitrary sockets 1245d1388b003c46092937def7041917aecec8de netbase: extend CreateSock() to support creating arbitrary sockets (Vasil Dimov) Pull request description: Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol. In addition to extending arguments, some extra safety checks were put in place. The need for this came up during the discussion in https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1618837102 ACKs for top commit: achow101: ACK 1245d1388b003c46092937def7041917aecec8de tdb3: re ACK 1245d1388b003c46092937def7041917aecec8de theStack: re-ACK 1245d1388b003c46092937def7041917aecec8de Tree-SHA512: cc86b56121293ac98959aed0ed77812d20702ed7029b5a043586f46e74295779c5354bb0d5f9e80be6c29e535df980d34c1dbf609064fb7ea3e5ca0f0ed54d6b
2024-06-20Merge bitcoin/bitcoin#29862: test: Validate oversized transactions or ↵Ava Chow
without inputs 969e047cfbab86e5819a2c9056e8d2dab17513a8 Replace hard-coded constant in test (Lőrinc) 327a31d1a4f0e9c7b22063bc725bbd160092c552 Validate oversized transaction (Lőrinc) 1984187840972a455f4c210f0cb576633ef5bddb Validate transaction without inputs (Lőrinc) c3a884318981c7ebabd0b8e8023a14519e26c72b Use SCRIPT_VERIFY_NONE instead of hard-coded 0 in transaction_tests (Lőrinc) Pull request description: Based on https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_check.cpp.gcov.html empty inputs and oversized transactions weren't covered by Boost unit tests (though they're covered by [python](https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_accept.py#L231) [tests](https://github.com/bitcoin/bitcoin/blob/master/test/functional/data/invalid_txs.py#L102)). <img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/57a74ff5-5466-401f-a4fe-d79e36964adf"> I have tried including the empty transaction into [tx_invalid.json](https://github.com/bitcoin/bitcoin/blob/master/src/test/data/tx_invalid.json#L34-L36), but it failed for another reason, so I added a separate test case for it in the end. The oversized tx data is on the failure threshold now (lower threshold fails for a different reason, but I guess that's fine, we're testing the boundary here). ACKs for top commit: achow101: ACK 969e047cfbab86e5819a2c9056e8d2dab17513a8 tdb3: ACK 969e047cfbab86e5819a2c9056e8d2dab17513a8 pending `MSan, depends` CI failure. glozow: utACK 969e047cfbab86e5819a2c9056e8d2dab17513a8 Tree-SHA512: 2a472690eabfdacc276b7e0414d3a4ebc75c227405b202c9fe3c8befad875f6e4d9b40c056fb05971ad3ae479c8f53edebb2eeeb700088856caf5cf58bfca0c1
2024-06-20Merge bitcoin/bitcoin#29575: net_processing: make any misbehavior trigger ↵Ava Chow
immediate discouragement 6eecba475efd025eb011400af58621ad5823994e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille) ae60d485da33f238ed2186799da4e109d4edd3a1 net_processing: remove Misbehavior score and increments (Pieter Wuille) 6457c311977bba3585648e32e3bd5754028aa292 net_processing: make all Misbehaving increments = 100 (Pieter Wuille) 5120ab1478c200b18ee621a6ffa0362f4e991959 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille) 944c54290d5c081dc433dae7e7941074a3a8b5a7 net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille) 9f66ac7cf1931c4d7c36abbb000b7de306d83a4c net_processing: do not treat non-connecting headers as response (Pieter Wuille) Pull request description: So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation. This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary. The specific types of misbehavior that are changed to 100 are: * Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10] * Sending us a `headers` with a non-continuous headers sequence. [used to be score 20] * Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20] * Sending us more than 50000 invs in a single `inv` message [used to be score 20] * Sending us more than 2000 headers in a single `headers` message [used to be score 20] The specific types of misbehavior that are changed to 0 are: * Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20] * Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10] I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate. In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement. ACKs for top commit: sr-gi: ACK [6eecba4](https://github.com/bitcoin/bitcoin/pull/29575/commits/6eecba475efd025eb011400af58621ad5823994e) achow101: ACK 6eecba475efd025eb011400af58621ad5823994e mzumsande: Code Review ACK 6eecba475efd025eb011400af58621ad5823994e glozow: light code review / concept ACK 6eecba475efd025eb011400af58621ad5823994e Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
2024-06-20ci: add option for running tests without volumeMax Edwards
DANGER_CI_ON_HOST_CACHE_FOLDERS if set will mount caches in directories on the host rather than in docker volumes. Supports saving and restoring caches on Github Actions.
2024-06-20contrib: add R(UN)PATH check to ELF symbol-checkfanquake
Our binaries shouldn't contains any rpaths, or runpaths, so check that at release time.
2024-06-20Merge bitcoin/bitcoin#30307: fuzz: Fix wallet_bdb_parser 32-bit unhandled ↵merge-script
fseek error fa7bc9bbca9348cf31b97bee0789ea7caeec635c fuzz: Fix wallet_bdb_parser 32-bit unhandled fseek error (MarcoFalke) Pull request description: `std::fseek` on 64-bit past the end of the file may work fine (the following read would fail). However, on 32-bit it may fail early. Fix it, by ignoring the error, treating it similar to a read error. This was found by OSS-Fuzz. https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69414 ACKs for top commit: TheCharlatan: ACK fa7bc9bbca9348cf31b97bee0789ea7caeec635c brunoerg: utACK fa7bc9bbca9348cf31b97bee0789ea7caeec635c Tree-SHA512: 7a752a005837bae6846ce315a7b3b1a5fe1f440c7797c750f2c0bbb20b1ef1537cd390c425747c0c85d012655e2f908bd300ea084f82e5ada19badbf826e1ec9
2024-06-20Merge bitcoin/bitcoin#30248: refactor: Add explicit cast to ↵merge-script
expected_last_page to silence fuzz ISan fa9cb101cf33b57b2c043b29f1f3d55b990ba4c6 refactor: Add explicit cast to expected_last_page to silence fuzz ISan (MarcoFalke) Pull request description: Fixes #30247 I don't think this implicit cast can lead to any bugs, so make it explicit to silence the fuzz integer sanitizer. Can be tested with: ``` FUZZ=wallet_bdb_parser UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/1376869be72eebcc87fe737020add634b1a29533 ``` After downloading the raw fuzz input from https://github.com/bitcoin-core/qa-assets/blob/24c507b3ea6263e6b121fb8dced01123065c44c2/fuzz_seed_corpus/wallet_bdb_parser/1376869be72eebcc87fe737020add634b1a29533 ACKs for top commit: dergoegge: utACK fa9cb101cf33b57b2c043b29f1f3d55b990ba4c6 Tree-SHA512: 226dcc58be8d70b4eec1657f232c9c6648b5dac5eb2706e7390e65ce0a031fbaf8afce97d71a535c8294467dca4757c96f294d8cc03d5e6a1c0a036b0e070325
2024-06-19test: Make blockencodings_tests deterministicAngusP
refactor: CBlockHeaderAndShortTxIDs constructor now always takes an explicit nonce. test: Make blockencodings_tests deterministic using fixed seed providing deterministic CBlockHeaderAndShortTxID nonces and dummy transaction IDs. Fixes very rare flaky test failures, where the ShortIDs of test transactions collide, leading to `READ_STATUS_FAILED` from PartiallyDownloadedBlock::InitData and/or `IsTxAvailable` giving `false` when the transaction should actually be available. * Use a new `FastRandomContext` with a fixed seed in each test, to ensure 'random' uint256s used as fake prevouts are deterministic, so in-turn test txids and short IDs are deterministic and don't collide causing very rare but flaky test failures. * Add new test-only/internal initializer for `CBlockHeaderAndShortTxIDs` that takes a specified nonce to further ensure determinism and avoid rare but undesireable short ID collisions. In a test context this nonce is set to a fixed known-good value. Normally it is random, as previously. Flaky test failures can be reproduced with: ```patch diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 695e8d806a..64d635a97a 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const { uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const { static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids calculation assumes 6-byte shorttxids"); - return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL; + // return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL; + return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0x0f; } ``` to increase the likelihood of a short ID collision; and running ```shell set -e; n=0; while (( n++ < 5000 )); do src/test/test_bitcoin --run_test=blockencodings_tests; done ```
2024-06-19QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core"Luke Dashjr
2024-06-19Merge bitcoin/bitcoin#30300: fuzz: have package_rbf always make small txnsglozow
4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe fuzz: have package_rbf always make small txns (Greg Sanders) Pull request description: hopefully resolves https://github.com/bitcoin/bitcoin/issues/30241 The fuzz target is generating a large amount of transactions, but the core of the logic is ConsumeTxMemPoolEntry making the mempool entries for adding to the mempool. Since ConsumeTxMemPoolEntry generates its own transaction "vsize", we can improve efficiency of the target by explicitly creating very small transactions, reducing the hashing and memory burden. ACKs for top commit: maflcko: lgtm ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe hodlinator: ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe glozow: ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe Tree-SHA512: 5d2e7e98460c6144dfe7deac554865e2e8e0e5f934dbdf5857dc4b4f471a64dc933297dc0dcf516f748a4348be6bd184808b7ece17ce073fdcc77f81b74c64de
2024-06-19fuzz: Fix wallet_bdb_parser 32-bit unhandled fseek errorMarcoFalke
2024-06-19Don't use iterator addresses in IteratorComparatordergoegge
The addresses of the iterator values are non-deterministic (i.e. they depend on where the values were allocated). This causes stability issues when fuzzing (e.g. in the `txorphan` and `mini_miner` harnesses), due the orders (derived from IteratorComparator) not being deterministic. Improve stability by comparing the first element in the iterator value pair instead of using the the value addresses.
2024-06-19Merge bitcoin/bitcoin#30283: upnp: fix build with miniupnpc 2.2.8merge-script
8acdf66540834b9f9cf28f16d389e8b6a48516d5 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields) Pull request description: Fixes https://github.com/bitcoin/bitcoin/issues/30266 Miniupnpc 2.2.8 [changed the function signature of `UPNP_GetValidIGD`](https://github.com/miniupnp/miniupnp/commit/c0a50ce33e3b99ce8a96fd43049bb5b53ffac62f#diff-5a0d7cff00628c2c64a617edb347c0f283e3a75e7df910e7e8438fc6db23f610R122) without taking much care with the abi :( ~This is the minimal change to cope with that. Also included in this PR is a temporary bump to 2.2.8 to verify that it builds correctly. I'm happy to revert that and discuss the bump separately, as miniupnpc bumps require some scrutiny.~ I believe that this is problematic if we build against one version and encounter a different one at runtime. This is not a problem for depends because we build statically. But for users who are self-building against shared system libs, care must be taken to run against the same version used for linking. Some quick digging shows that at least Ubuntu/Arch make the distinction between soversions: `libminiupnpc.so.17` -> `libminiupnpc.so.18`. So in practice, I suppose this shouldn't be much of a problem. Boooo for the upstream loose abi policy. ACKs for top commit: edilmedeiros: reACK 8acdf66540834b9f9cf28f16d389e8b6a48516d5 fanquake: ACK 8acdf66540834b9f9cf28f16d389e8b6a48516d5 Tree-SHA512: d2236ec8aef57a5c879065fbbe20080a14e4bf7b44c0bf506707eb946f72aa5837aba2fb2426d6853d21a9b77db5d72561d29d7ea645714d90309e11fe11d354
2024-06-18rpc: call IsInitialBlockDownload via miner interfaceSjors Provoost
2024-06-18Replace hard-coded constant in testLőrinc
2024-06-18Validate oversized transactionLőrinc
2024-06-18Validate transaction without inputsLőrinc
2024-06-18Use SCRIPT_VERIFY_NONE instead of hard-coded 0 in transaction_testsLőrinc
2024-06-18rpc: minize getTipHash() calls in gbtSjors Provoost
Set tip at the start of the function and only update it for a long poll. Additionally have getTipHash return an optional, so the caller can explicitly check that a tip exists.
2024-06-18rpc: call processNewBlock via miner interfaceSjors Provoost
2024-06-18rpc: getTransactionsUpdated via miner interfaceSjors Provoost
2024-06-18Always pass options to BlockAssembler constructorSjors Provoost
This makes the options argument for BlockAssembler constructor mandatory, dropping implicit use of ArgsManager. The caller i.e. the Mining interface implementation now handles this. In a future Stratum v2 change the Options object needs to be mofified after arguments have been processed. Specifically the pool communicates how many extra bytes it needs for its own outputs (payouts, extra commitments, etc). This will need to be substracted from what the user set as -blockmaxweight. Such a change can be implemented in createNewBlock, after ApplyArgsManOptions.
2024-06-18rpc: call CreateNewBlock via miner interfaceSjors Provoost
2024-06-18rpc: getblocktemplate getTipHash() via Miner interfaceSjors Provoost
2024-06-18rpc: call TestBlockValidity via miner interfaceSjors Provoost
2024-06-18Introduce Mining interfaceSjors Provoost
Start out with a single method isTestChain() that's used by the getblocktemplate RPC.
2024-06-18fuzz: have package_rbf always make small txnsGreg Sanders
The fuzz target is generating a large amount of transactions, but the core of the logic is ConsumeTxMemPoolEntry making the mempool entries for adding to the mempool. Since ConsumeTxMemPoolEntry generates its own transaction "vsize", we can improve efficiency of the target by explicitly creating very small transactions, reducing the hashing and memory burden.
2024-06-18upnp: add compatibility for miniupnpc 2.2.8Cory Fields
See: https://github.com/miniupnp/miniupnp/commit/c0a50ce33e3b99ce8a96fd43049bb5b53ffac62f The return value of 2 now indicates: "A valid connected IGD has been found but its IP address is reserved (non routable)" We continue to ignore any return value other than 1.
2024-06-18Merge bitcoin/bitcoin#30287: macOS: rewrite some docs & swap ↵merge-script
`mmacosx-version-min` for `mmacos-version-min` 7c298fe0df38696f60e981469422315c03a722da doc: rewrite some of the macdeploy docs (fanquake) d042230f7a7ada03f85cc392b8f2c4b56e779d61 depends: swap mmacosx-version-min for mmacos-version-min (fanquake) Pull request description: Whilst `-mmacosx-version-min` and `-mmacos-version-min` remain aliases for each other, the later is preferred, and I assume the former will be removed at some point in the future; see: https://github.com/llvm/llvm-project/pull/95374. Somewhat of a followup to #21778. Rewrite some of the mac deploy docs. ACKs for top commit: theuni: utACK 7c298fe0df38696f60e981469422315c03a722da TheCharlatan: ACK 7c298fe0df38696f60e981469422315c03a722da hebasto: ACK 7c298fe0df38696f60e981469422315c03a722da. Tree-SHA512: 6493f087fde93e0eec319af0e105d163b3f047d8a03f7d4b0d6cd7c64b58d0a978b7d67c6b8dba5c6ccf8b10e188aab5dc98eec400b0546dc9ee801a689b4332
2024-06-18Merge bitcoin/bitcoin#30299: ci: remove unused bcc variable from workflowmerge-script
518b06c4b889d71a3fdd61f8fe38d519ea5e4a1b ci: remove unused bcc variable from workflow (Max Edwards) Pull request description: Fixes https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1636804002 ACKs for top commit: maflcko: lgtm ACK 518b06c4b889d71a3fdd61f8fe38d519ea5e4a1b Tree-SHA512: c87364670e26e15176ee21372a2cc100db0c275a5cffb37cc33ec4c2d85d6067b593bd4a6dea37bf478d2af197786df9dfac3cfb76db023c8db37184bb104458
2024-06-18Merge bitcoin/bitcoin#30282: Revert "contrib: macdeploy: monkey-patch ↵merge-script
gen-sdk to be deterministic" b03a45b13e4e33b044cae6c97a6d608f6f3d43f3 Revert "contrib: macdeploy: monkey-patch gen-sdk to be deterministic" (fanquake) Pull request description: This reverts commit ba30a5407e065e9d6dd037351e83f56a43f38f19. We no-longer support Python 3.8, so remove the monkey patching. ACKs for top commit: hebasto: ACK b03a45b13e4e33b044cae6c97a6d608f6f3d43f3, I have reviewed the code and it looks OK. Tree-SHA512: 5bf68c2b332f18a620a8a6f77812ed93afa988016847bec1d3b7355670301dc957442ac47191a0cb7c3fe607d902914fb00c96345c8170f2a64429638c00b3c4
2024-06-18ci: remove unused bcc variable from workflowMax Edwards
2024-06-17Merge bitcoin/bitcoin#28984: Cluster size 2 package rbfAva Chow
94ed4fbf8e1a396c650b5134d396d6c0be35ce10 Add release note for size 2 package rbf (Greg Sanders) afd52d8e63ed323a159ea49fd1f10542abeacb97 doc: update package RBF comment (Greg Sanders) 6e3c4394cfadf32c06c8c4732d136ca10c316721 mempool: Improve logging of replaced transactions (Greg Sanders) d3466e4cc5051c314873dd14ec8f7a88494c0780 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders) 316d7b63c97144ba3e21201315c784852210f8ff Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders) 4d15bcf448eb3c4451b63e8f78cc61f3f9f9b639 [test] package rbf (glozow) dc21f61c72e5a97d974ca2c5cb70b8328f4fab2a [policy] package rbf (Suhas Daftuar) 5da396781589177d4ceb3b4b59c9f309a5e4d029 PackageV3Checks: Relax assumptions (Greg Sanders) Pull request description: Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less. Proposed validation steps: 1) If the transaction package is of size 1, legacy rbf rules apply. 2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail. 3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5) 4) The package is a single chunk 5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2). 6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool 7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4) Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today. ACKs for top commit: achow101: ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10 glozow: reACK 94ed4fbf8e via range-diff ismaelsadeeq: re-ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10 theStack: Code-review ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10 murchandamus: utACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10 Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
2024-06-17Merge bitcoin/bitcoin#30058: Encapsulate warnings in generalized ↵Ava Chow
node::Warnings and remove globals 260f8da71a35232d859d7705861fc1a88bfbbe81 refactor: remove warnings globals (stickies-v) 9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f node: update uiInterface whenever warnings updated (stickies-v) b071ad9770b7ae7fc718dcbfdc8f62dffbf6cfee introduce and use the generalized `node::Warnings` interface (stickies-v) 20e616f86444d00712ac7eb840666e2b0378af4a move-only: move warnings from common to node (stickies-v) bed29c481aebeb2b0160450c63c03cc68fb89bc6 refactor: remove unnecessary AppendWarning helper function (stickies-v) Pull request description: This PR: - moves warnings from common to the node library and into the node namespace (as suggested in https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1570069541) - generalizes the warnings interface to `Warnings::Set()` and `Warnings::Unset()` methods, instead of having a separate function and globals for each warning. As a result, this simplifies the `kernel::Notifications` interface. - removes warnings.cpp from the kernel library - removes warning globals - adds testing for the warning logic Behaviour change introduced: - the `-alertnotify` command is executed for all `KernelNotifications::warningSet` calls, which now also covers the `LARGE_WORK_INVALID_CHAIN` warning - the GUI is updated automatically whenever a warning is (un)set, covering some code paths where it previously wouldn't be, e.g. when `node::AbortNode()` is called, or for the `LARGE_WORK_INVALID_CHAIN` warning Some discussion points: - ~is `const std::string& id` the best way to refer to warnings? Enums are an obvious alternative, but since we need to define warnings across libraries, strings seem like a straightforward solution.~ _edit: updated approach to use `node::Warning` and `kernel::Warning` enums._ ACKs for top commit: achow101: ACK 260f8da71a35232d859d7705861fc1a88bfbbe81 ryanofsky: Code review ACK 260f8da71a35232d859d7705861fc1a88bfbbe81. Only change since last review was rebasing TheCharlatan: Re-ACK 260f8da71a35232d859d7705861fc1a88bfbbe81 Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588
2024-06-17Merge bitcoin/bitcoin#30193: ci: move ASan job to GitHub Actions from Cirrus CIAva Chow
9eea51d9058ad638861aa4b94c1c6e71caeb8765 ci: move Asan / LSan / USDT job to Github Actions (Max Edwards) 4b527fa93b9763a33842069bc07446313cbf5e0f ci: add IPV6 network to ci container (Max Edwards) Pull request description: PR for moving the ASAN + LSAN + USDT + friends job to github actions from Cirrus. The motivation for this PR is that this task needs a full VM (or bare metal) to function, because of the tracepoints. It can not run in a container on an arbitrary Linux, because the outside machine must exactly match the specification of the distro used in the CI task config. This requires more maintenance for the persistent worker, and I think moving to GHA will reduce the maintenance burden, or at least make it possible for anyone to work on. Also, it makes it easier to run the task on forks (bitcoin-inquisition, bitcoin-knots, devel forks, ...) without having to set-up a real machine. ACKs for top commit: maflcko: review ACK 9eea51d9058ad638861aa4b94c1c6e71caeb8765 achow101: ACK 9eea51d9058ad638861aa4b94c1c6e71caeb8765 hebasto: ACK 9eea51d9058ad638861aa4b94c1c6e71caeb8765. Tree-SHA512: 1111c1c9e3a11e725dff1344643fff3c91fb9b4d7c1cc9a7d507a8f146f5223316a00272030b41ae37ecb59d044f2e90e1cd907450049b25f094f0b60643d4c7
2024-06-17Merge bitcoin/bitcoin#30195: test: Added test coverage to listsinceblock rpcAva Chow
881724d443d11f984a721ef1edd5777c24d1ed29 test: Added test coverage to listsinceblock rpc (kevkevinpal) Pull request description: This change is meant to add test coverage to this rpc error https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L666C53-L666C79 This is done by renaming the first block in the blocks folder --- Doing a quick grep for the error code in our functional tests leads to zero results `grep -nri "Can't read block from disk" ./test/functional/` ACKs for top commit: achow101: ACK 881724d443d11f984a721ef1edd5777c24d1ed29 tdb3: re ACK for 881724d443d11f984a721ef1edd5777c24d1ed29 rkrux: tACK [881724](https://github.com/bitcoin/bitcoin/pull/30195/commits/881724d443d11f984a721ef1edd5777c24d1ed29) Tree-SHA512: c5dff20cf014d0181f49d6b161f1364e1c6b79e8661047f77f07e21e59f4d1f2fd6f745538c8fc5bd6d4244650a840dd64d184634366f7c21fa67141a60af44a
2024-06-17Merge bitcoin/bitcoin#28312: test: fix `keys_to_multisig_script` (P2MS) ↵Ava Chow
helper for n/k > 16 5cf0a1f230389ef37e0ff65de5fc98394f32f60c test: add `createmultisig` P2MS encoding test for all n (1..20) (Sebastian Falbesoner) 0570d2c204ec7f10af6bd8e48c23318a48fefc10 test: add unit test for `keys_to_multisig_script` (Sebastian Falbesoner) 0c41fc3fa52ad16923afbd0ec18b9c1b3ded8036 test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16 (Sebastian Falbesoner) Pull request description: While reviewing #28307, I noticed that the test framework's `key_to_multisig_script` helper (introduced in #23305) is broken for pubkey count (n) and threshold (k) values larger than 16. This is due to the implementation currently enforcing a direct single-byte data push (using `CScriptOp.encode_op_n`), which obviously fails for values 17+. Fix that by passing the numbers directly to the CScript list, where it's automatically converted to minimally-encoded pushes (see class method `CScript.__coerce_instance`, branch `isinstance(other, int)`). The second commit adds a unit test to ensure that the encoding is correct. ACKs for top commit: achow101: ACK 5cf0a1f230389ef37e0ff65de5fc98394f32f60c tdb3: ACK 5cf0a1f230389ef37e0ff65de5fc98394f32f60c rkrux: reACK [5cf0a1f](https://github.com/bitcoin/bitcoin/pull/28312/commits/5cf0a1f230389ef37e0ff65de5fc98394f32f60c) Tree-SHA512: 4168a165c3f483ec8e37a27dba1628a7ea0063545a2b7e74d9e20d753fddd7e33d37e1a190434fa6dca39adf9eef5d0211f7a0c1c7b44979f0a3bb350e267562
2024-06-17Merge bitcoin/bitcoin#30291: test: write functional test results to csvAva Chow
ad06e68399da71c615db0dbf5304d0cd46bc1f40 test: write functional test results to csv (tdb3) Pull request description: Adds argument `--resultsfile` to test_runner.py. Enables functional test results to be written to a (csv) file for processing by other applications (or for historical archiving). Test name, status, and duration are written to the file provided with the argument. Since `test_runner.py` is being touched, also fixes a misspelling (linter warning). Can split into its own commit if desired. #### Notes - Total runtime of functional tests has seemed to have increased on my development machines over the past few months (more tests added, individual test runtime increase, etc.). Was interested in recording test runtime data over time to detect trends. Initially searched `doc/benchmarking.md`, existing PRs, and Issues, but didn't immediately see this type of capability or alternate solutions (please chime in if you know of one!). Thought it would be beneficial to add this capability to `test_runner` to facilitate this type of data analysis (and potentially other use cases) - Saw https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#benchmarking-with-perf, and this PR's higher level data seems complimentary. - Was on the fence as to whether to expand `print_results()` (i.e. take advantage of the same loop over `test_results`) or implement in a separate `write_results()` function. Decided on the latter for now, but interested in reviewers' thoughts. #### Example 1: all tests pass ``` $ test/functional/test_runner.py --resultsfile functional_test_results.csv --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp feature_blocksdir wallet_startup feature_config_args mempool_accept Temporary test directory at /mnt/tmp/test_runner_₿_🏃_20240614_201625 Test results will be written to functional_test_results.csv ... $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,29 mempool_accept.py,Passed,9 wallet_startup.py,Passed,2 ALL,Passed,29 ``` #### Example 2: one test failure ``` $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,28 wallet_startup.py,Passed,2 mempool_accept.py,Failed,1 ALL,Failed,28 ``` ACKs for top commit: maflcko: re-ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 kevkevinpal: tACK [ad06e68](https://github.com/bitcoin/bitcoin/pull/30291/commits/ad06e68399da71c615db0dbf5304d0cd46bc1f40) achow101: ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 rkrux: tACK [ad06e68](https://github.com/bitcoin/bitcoin/pull/30291/commits/ad06e68399da71c615db0dbf5304d0cd46bc1f40) brunoerg: ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 marcofleon: Good idea, tested ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 Tree-SHA512: 561194406cc744905518aa5ac6850c07c4aaecdaf5d4d8b250671b6e90093d4fc458f050e8a85374e66359cc0e0eaceba5eb24092c55f0d8f349d744a32ef76c
2024-06-17move-only: refactor CreateTransactionInternaljosibake
Move the output serialization size and dust calculation into the loop where the outputs are iterated over to calculate the total sum. Move the code for adding the the txoutputs to the transaction to after coin selection. While this code structure generally follows a more logical flow, the primary motivation for moving the code for adding outputs to the transaction sets us up nicely for silent payments (in a future PR): we need to know the input set before generating the final output scriptPubKeys.
2024-06-17wallet: use CRecipient instead of CTxOutjosibake
Now that a CRecipient holds a CTxDestination, we can get the serialized size and determine if the output is dust using the CRecipient directly. This does not change any current behavior, but provides a nice generalization that can be used to apply special logic to a CTxDestination serialization and dust calculations in the future. Specifically, in a later PR when support for `V0SilentPayment` destinations is added, we need to use `WitnessV1Taproot` as the scriptPubKey for serialized size calcuations whenever the `CRecipient` destination is a `V0SilentPayment` destination.
2024-06-16test: write functional test results to csvtdb3
Adds argument --resultsfile to test_runner.py. Writes comma-separated functional test name, status, and duration to the file provided with the argument. Also fixes minor typo in test_runner.py
2024-06-16test: Added test coverage to listsinceblock rpckevkevinpal
This change adds a test to add coverage to the rpc error that emmits the message "Can't read block from disk"
2024-06-14Merge bitcoin/bitcoin#27969: bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE ↵Ava Chow
when user specifies fee_rate f58beabe754363cb7d5b24032fd392654b9514ac test: bumpfee with user specified fee_rate ignores walletIncrementalRelayFee (ismaelsadeeq) 436e88f4336199998184cbfa5d1c889ffaefbfb5 bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate (ismaelsadeeq) Pull request description: Fixes #26973 When using the `bumpfee` RPC and manually specifying `fee_rate`, there should be no requirement that the new fee must be at least the sum of the original fee and `incrementalFee` (maximum of `relayIncrementalFee` and `WALLET_INCREMENTAL_RELAY_FEE`). This restriction should only apply when user did not specify `fee_rate`. > because the GUI doesn't let the user specify the new fee rate yet (https://github.com/bitcoin-core/gui/issues/647), it would be very annoying to have to bump 20 times to increment by 20 sat/vbyte. The restriction should instead be the new fee must be at least the sum of the original fee and `incrementalFee` (`relayIncrementalFee`) ACKs for top commit: achow101: ACK f58beabe754363cb7d5b24032fd392654b9514ac murchandamus: ACK f58beabe754363cb7d5b24032fd392654b9514ac Tree-SHA512: 193259f87173b7d5a8e68e0e29f2ca7e75c550e3cf0dee3d6d822b5b1e07c2e6dec0bfc8fb435855736ebced97a10dbdbfef72e8c5abde06fdefcba122f2e7f1
2024-06-14Merge bitcoin/bitcoin#30255: log: use error level for critical log messagesAva Chow
fae3a1f0065064d80ab4c0375a9eaeb666c5dd55 log: use error level for critical log messages (MarcoFalke) Pull request description: This picks up the first commit from https://github.com/bitcoin/bitcoin/pull/29231, but extends it to also cover cases that were missed in it. As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down. ACKs for top commit: stickies-v: re-ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55, I'm ~0 on the latest force push as `user_error` was already logged at the right level through `GetNotifications().fatalError(user_error);` so I'd be in favour of deduplicating/cleaning up this logging logic but can be done in follow-up. kevkevinpal: ACK [fae3a1f](https://github.com/bitcoin/bitcoin/pull/30255/commits/fae3a1f0065064d80ab4c0375a9eaeb666c5dd55) achow101: ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55 Tree-SHA512: 3f99fd25d5a204d570a42d8fb2b450439aad7685692f9594cc813d97253c4df172a6ff3cf818959bfcf25dfcf8ee9a9c9ccc6028fcfcecdb47591e18c77ef246
2024-06-14fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK readVasil Dimov
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`.
2024-06-14fuzz: simplify FuzzedSock::m_peek_dataVasil Dimov
`FuzzedSock::m_peek_data` need not be an optional of a vector. It can be just a vector whereas an empty vector denotes "no peek data".
2024-06-14netbase: extend CreateSock() to support creating arbitrary socketsVasil Dimov
Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol.
2024-06-14Merge bitcoin/bitcoin#30281: Update leveldb subtree to latest upstreammerge-script
a37778d4d32b4ddeff96f68a130dc8da3a84b278 Squashed 'src/leveldb/' changes from e2f10b4e47..688561cba8 (fanquake) Pull request description: Includes https://github.com/bitcoin-core/leveldb-subtree/pull/41 which is used in #30234. ACKs for top commit: theuni: utACK 95812d912b6335caa7af2a084d84447fb4aad156 Tree-SHA512: 3d943695a3d33816cf5558b183f5629aa92a500a1544eecedf84952e93c8592a8cf0d554b88281fc0bad3c9e920ebcff1ed8edc12f8e73f36ed5335482beb829
2024-06-14Merge bitcoin/bitcoin#30276: doc: archive release notes for v27.1merge-script
e71a21b641541f4751ac1ea7fd7d33f20a629636 doc: archive release notes for v27.1 (fanquake) Pull request description: ACKs for top commit: benalleng: ACK for e71a21b Tree-SHA512: a89231d09b442e5e1755135a475030648a81bb0c331eec8d9c7a27a633f64fd0449dcfe8939c0d43cc5133ad0ab85acda7de757594986f9c93fe6f90efc11a02
generated by cgit v1.2.3 (git 2.26.2) at 2024-12-18 02:08:42 +0000