💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582976807)
`GetFirstBlock()` does not provide any guarantees that `pprev` is not `nullptr` (nor should it), so I think we need to assert that here
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582976807)
`GetFirstBlock()` does not provide any guarantees that `pprev` is not `nullptr` (nor should it), so I think we need to assert that here
💬 brunoerg commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#issuecomment-2082585841)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29990#issuecomment-2082585841)
Concept ACK
💬 brunoerg commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1583009692)
> Considering the purpose of fuzzing is to test the combination of valid states, shouldn't we include tests that involve sending duplicate transactions to the mempool?
In this case, it is calling `addUnchecked` directly that's why it would be proper to check whether the transaction is in mempool before. Note that, in practice, this function is used in `MemPoolAccept::Finalize` which removes conflicting transactions from the mempool before adding.
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1583009692)
> Considering the purpose of fuzzing is to test the combination of valid states, shouldn't we include tests that involve sending duplicate transactions to the mempool?
In this case, it is calling `addUnchecked` directly that's why it would be proper to check whether the transaction is in mempool before. Note that, in practice, this function is used in `MemPoolAccept::Finalize` which removes conflicting transactions from the mempool before adding.
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583018329)
In some cases, tracepoint argument preparation can require more than a single readable line in the `TRACEPOINT` macro. An example is the transaction serialization [here](https://github.com/0xB10C/bitcoin/blob/6464cc95ca97c4535de79e52ed174e58ea0bfb18/src/txmempool.cpp#L467-L478). For context, see https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1333832906. I don't currently plan on PRing something like this, but had this running for more than a year to collect data on full-rbf RBF repla
...
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583018329)
In some cases, tracepoint argument preparation can require more than a single readable line in the `TRACEPOINT` macro. An example is the transaction serialization [here](https://github.com/0xB10C/bitcoin/blob/6464cc95ca97c4535de79e52ed174e58ea0bfb18/src/txmempool.cpp#L467-L478). For context, see https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1333832906. I don't currently plan on PRing something like this, but had this running for more than a year to collect data on full-rbf RBF repla
...
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583019336)
I agree that adding this makes sense as the bcc error message isn't clear. If I retouch, I'll add it.
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583019336)
I agree that adding this makes sense as the bcc error message isn't clear. If I retouch, I'll add it.
💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583020605)
Ah I see, that makes sense now then, thanks.
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583020605)
Ah I see, that makes sense now then, thanks.
🤔 furszy reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2028393990)
ACK 9552619961049d7673f84c40917b0385be70b782
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2028393990)
ACK 9552619961049d7673f84c40917b0385be70b782
💬 instagibbs commented on pull request "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py":
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1583032012)
```suggestion
fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"] * COIN))
```
would you consider this instead? seems the more direct thing we're trying to accomplish
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1583032012)
```suggestion
fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"] * COIN))
```
would you consider this instead? seems the more direct thing we're trying to accomplish
👍 hebasto approved a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2028426491)
ACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8.
I suggest to consider updating a help string in https://github.com/bitcoin/bitcoin/blob/a46065e36cf868265c909dc5edf29dc17be53c1f/configure.ac#L1133 to "Define to 1 if O_CLOEXEC flag is available, and to 0 if not."
As a follow up, the https://github.com/bitcoin/bitcoin/pull/16344 might be reconsidered.
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2028426491)
ACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8.
I suggest to consider updating a help string in https://github.com/bitcoin/bitcoin/blob/a46065e36cf868265c909dc5edf29dc17be53c1f/configure.ac#L1133 to "Define to 1 if O_CLOEXEC flag is available, and to 0 if not."
As a follow up, the https://github.com/bitcoin/bitcoin/pull/16344 might be reconsidered.
👍 fanquake approved a pull request: "build: Bump clang minimum supported version to 15"
(https://github.com/bitcoin/bitcoin/pull/29165#pullrequestreview-2028437917)
ACK fa1964c5b80ee28b0e06abdbd9a26e8e8c6f5acd - oss-fuzz LLVM will either be bumped globally tomorrow, or we'll land our own bump.
(https://github.com/bitcoin/bitcoin/pull/29165#pullrequestreview-2028437917)
ACK fa1964c5b80ee28b0e06abdbd9a26e8e8c6f5acd - oss-fuzz LLVM will either be bumped globally tomorrow, or we'll land our own bump.
🚀 fanquake merged a pull request: "build: Bump clang minimum supported version to 15"
(https://github.com/bitcoin/bitcoin/pull/29165)
(https://github.com/bitcoin/bitcoin/pull/29165)
📝 willcl-ark opened a pull request: "gui: fix misleading signmessage error with segwit"
(https://github.com/bitcoin-core/gui/pull/819)
As described in https://github.com/bitcoin/bitcoin/issues/10542 (and numerous other places), message signing in Bitcoin Core does not support "signing with a segwit address" and likely will not in the foreseeable future, or at least until a new message-signing standard is agreed upon.
Therefore update the possibly misleading error message presented to the user in the GUI to detail more specifically the reason their message cannot be signed, in the case that a non P2PKH address is entered.
...
(https://github.com/bitcoin-core/gui/pull/819)
As described in https://github.com/bitcoin/bitcoin/issues/10542 (and numerous other places), message signing in Bitcoin Core does not support "signing with a segwit address" and likely will not in the foreseeable future, or at least until a new message-signing standard is agreed upon.
Therefore update the possibly misleading error message presented to the user in the GUI to detail more specifically the reason their message cannot be signed, in the case that a non P2PKH address is entered.
...
💬 0xB10C commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2082675302)
I was made aware of https://bencher.dev/learn/engineering/sqlite-performance-tuning/ recently and the [dashboard](https://bencher.dev/perf/0xb10c-s-bitcoin-core?key=true&reports_per_page=4&branches_per_page=8&testbeds_per_page=8&benchmarks_per_page=8&reports_page=1&branches_page=1&testbeds_page=1&benchmarks_page=1&report=915c1007-33dd-4d30-b9c1-531b60af8a06&branches=0ab6caf7-716b-4a6e-8101-f2b9c891a9a2&testbeds=3c0c4d9d-b40e-4d9a-a462-2379355b0785&benchmarks=993b5444-b391-43dd-a4c1-c157cb1d7209%
...
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2082675302)
I was made aware of https://bencher.dev/learn/engineering/sqlite-performance-tuning/ recently and the [dashboard](https://bencher.dev/perf/0xb10c-s-bitcoin-core?key=true&reports_per_page=4&branches_per_page=8&testbeds_per_page=8&benchmarks_per_page=8&reports_page=1&branches_page=1&testbeds_page=1&benchmarks_page=1&report=915c1007-33dd-4d30-b9c1-531b60af8a06&branches=0ab6caf7-716b-4a6e-8101-f2b9c891a9a2&testbeds=3c0c4d9d-b40e-4d9a-a462-2379355b0785&benchmarks=993b5444-b391-43dd-a4c1-c157cb1d7209%
...
💬 sdaftuar commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1583059571)
> In this case, it is calling addUnchecked directly that's why it would be proper to check whether the transaction is in mempool before. Note that, in practice, this function is used in MemPoolAccept::Finalize which removes conflicting transactions from the mempool before adding.
Yes exactly. The correct interface for code that is doing no sanity checking would be `AcceptToMemoryPool()`. Since we're bypassing that in the fuzz tests, we should do something else to avoid putting the mempool in
...
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1583059571)
> In this case, it is calling addUnchecked directly that's why it would be proper to check whether the transaction is in mempool before. Note that, in practice, this function is used in MemPoolAccept::Finalize which removes conflicting transactions from the mempool before adding.
Yes exactly. The correct interface for code that is doing no sanity checking would be `AcceptToMemoryPool()`. Since we're bypassing that in the fuzz tests, we should do something else to avoid putting the mempool in
...
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583065711)
looked a bit closer, how is this different from the "# ...even if it's submitted with other transactions" case above in the test? To check package rbf the only difference is making sure it's evaluated as a single subpackage(by having the child feerate be larger)
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583065711)
looked a bit closer, how is this different from the "# ...even if it's submitted with other transactions" case above in the test? To check package rbf the only difference is making sure it's evaluated as a single subpackage(by having the child feerate be larger)
💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583082284)
Ahh I see, it's the same code path.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583082284)
Ahh I see, it's the same code path.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583085742)
oh right, neither of these cases will do a package rbf, as the package has in-mempool ancestors.
It would have to be a scenario where the ancestor limits are set to 1, then a package RBF of size 2 replaces that single tx?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583085742)
oh right, neither of these cases will do a package rbf, as the package has in-mempool ancestors.
It would have to be a scenario where the ancestor limits are set to 1, then a package RBF of size 2 replaces that single tx?
💬 hebasto commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2082750970)
How is it supposed to automatically catch cases when code changes make `#include <config/bitcoin-config.h>` unneeded in a header or source file?
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2082750970)
How is it supposed to automatically catch cases when code changes make `#include <config/bitcoin-config.h>` unneeded in a header or source file?
🚀 fanquake merged a pull request: "test: Add missing Assert(mock_time_in >= 0s) to SetMockTime"
(https://github.com/bitcoin/bitcoin/pull/29872)
(https://github.com/bitcoin/bitcoin/pull/29872)
💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583113813)
Hmm, I think it will always be the same code path that will be hit because the parent transaction (in the cluster size 2 replacement package) will be detected as conflict with the transaction that was carved out (because it has to go through `PreChecks`), and we don't allow package RBF carveout, so the subpackage evaluation will not be executed?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1583113813)
Hmm, I think it will always be the same code path that will be hit because the parent transaction (in the cluster size 2 replacement package) will be detected as conflict with the transaction that was carved out (because it has to go through `PreChecks`), and we don't allow package RBF carveout, so the subpackage evaluation will not be executed?