💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171607771)
👍 this is where the test fails correctly without the `!CheckSigopsBIP54(tx, mapInputs)` check.
Would it make sense to add a coinbase tx and vouts to this test to make it more realistic so that it passes `CheckTransaction` as well? Otherwise it's harder to tell if it fails for the right reasons...
And to make sure this only affects policy and not consensus: do we already have tests that check that e.g. 3000 sigops could still be mined? If not, can we add a clause here to make sure that we
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171607771)
👍 this is where the test fails correctly without the `!CheckSigopsBIP54(tx, mapInputs)` check.
Would it make sense to add a coinbase tx and vouts to this test to make it more realistic so that it passes `CheckTransaction` as well? Otherwise it's harder to tell if it fails for the right reasons...
And to make sure this only affects policy and not consensus: do we already have tests that check that e.g. 3000 sigops could still be mined? If not, can we add a clause here to make sure that we
...
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171712231)
In most docs I saw (e.g. https://learnmeabitcoin.com/technical/script/p2sh/#scriptsig), P2SH starts with an `OP_0` - I understand that this corresponds to an empty vector, but maybe we can simplify it in the tests:
```suggestion
CScript max_sigops_redeem_script{CScript() << OP_0 << key.GetPubKey()};
```
<details>
<summary>validation</summary>
```C++
CScript max_sigops_redeem_script{CScript() << std::vector<unsigned char>{} << key.GetPubKey()};
CScript max_sigops_redeem_sc
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171712231)
In most docs I saw (e.g. https://learnmeabitcoin.com/technical/script/p2sh/#scriptsig), P2SH starts with an `OP_0` - I understand that this corresponds to an empty vector, but maybe we can simplify it in the tests:
```suggestion
CScript max_sigops_redeem_script{CScript() << OP_0 << key.GetPubKey()};
```
<details>
<summary>validation</summary>
```C++
CScript max_sigops_redeem_script{CScript() << std::vector<unsigned char>{} << key.GetPubKey()};
CScript max_sigops_redeem_sc
...
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171873609)
It's not immediately obvious that this is reducing the size of the existing inputs (especially since in other cases we did `reserve`), in this case maybe we could:
```suggestion
tx_max_sigops.vin.pop_back();
assert(tx_max_sigops.vin.size() == p2sh_inputs_count);
```
or add a comment here
```suggestion
// Drop the extra inputs
tx_max_sigops.vin.resize(p2sh_inputs_count);
```
or do the deletion explicitly:
```suggestion
tx_max_sigops.vin.erase(tx_max_sigops.vin.begi
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171873609)
It's not immediately obvious that this is reducing the size of the existing inputs (especially since in other cases we did `reserve`), in this case maybe we could:
```suggestion
tx_max_sigops.vin.pop_back();
assert(tx_max_sigops.vin.size() == p2sh_inputs_count);
```
or add a comment here
```suggestion
// Drop the extra inputs
tx_max_sigops.vin.resize(p2sh_inputs_count);
```
or do the deletion explicitly:
```suggestion
tx_max_sigops.vin.erase(tx_max_sigops.vin.begi
...
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171495102)
nit: if we want better error messages showing both sides on a failure, we could do:
```suggestion
BOOST_CHECK_GT((p2sh_inputs_count + 1) * MAX_P2SH_SIGOPS, MAX_TX_LEGACY_SIGOPS);
```
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171495102)
nit: if we want better error messages showing both sides on a failure, we could do:
```suggestion
BOOST_CHECK_GT((p2sh_inputs_count + 1) * MAX_P2SH_SIGOPS, MAX_TX_LEGACY_SIGOPS);
```
💬 rkrux commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#discussion_r2171959858)
I have used this suggestion in the `getbalances` RPC, retained original (to some extent) in the `getbalance` one because it aligns closely with the current verbiage around the term "spendable".
(https://github.com/bitcoin/bitcoin/pull/32761#discussion_r2171959858)
I have used this suggestion in the `getbalances` RPC, retained original (to some extent) in the `getbalance` one because it aligns closely with the current verbiage around the term "spendable".
💬 fanquake commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3012971337)
@purpleKarrot want to take another look here?
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3012971337)
@purpleKarrot want to take another look here?
💬 Eunovo commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3012975690)
ACK https://github.com/bitcoin/bitcoin/pull/32761/commits/cb44680db40c85ba338f2a054511cd68ba6e89ba
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3012975690)
ACK https://github.com/bitcoin/bitcoin/pull/32761/commits/cb44680db40c85ba338f2a054511cd68ba6e89ba
💬 purpleKarrot commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3012982665)
ACK 14653b869b91f8013656099c9eb23b3518b8e53e
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3012982665)
ACK 14653b869b91f8013656099c9eb23b3518b8e53e
💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013037586)
Side-note about when to call `LimitOrphans`: IIUC, so far the discussed scenarios to exceed the global limit were focused on the obvious cases of increasing the nominator of the DoS score fraction, i.e. adding new orphans or announcements (`AddTx`/`AddAnnouncement`). What about decreasing the denominator? Disconnecting a peer decreases the global usage limit, so if remaining peers have previously exceeded their per-peer usage limit, it could lead to an exceeding of global usage limit if the disc
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013037586)
Side-note about when to call `LimitOrphans`: IIUC, so far the discussed scenarios to exceed the global limit were focused on the obvious cases of increasing the nominator of the DoS score fraction, i.e. adding new orphans or announcements (`AddTx`/`AddAnnouncement`). What about decreasing the denominator? Disconnecting a peer decreases the global usage limit, so if remaining peers have previously exceeded their per-peer usage limit, it could lead to an exceeding of global usage limit if the disc
...
🤔 janb84 reviewed a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2966554130)
Concept ACK 40d935904c2b1319b863306d32fef235cd008a8e
(if we want to keep python check in CMAKE)
PR Adds build option to ignore / not fail on missing python. When python is missing or below required version the configuration will error out. This removes failing silently on python.
This PR will partial solve issue #31476 (strictly the Python issue)
---
Alternative thoughts:
Should CMAKE even test for python ? If you see CMAKE as configuration for the BUILD step and we do not use pytho
...
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2966554130)
Concept ACK 40d935904c2b1319b863306d32fef235cd008a8e
(if we want to keep python check in CMAKE)
PR Adds build option to ignore / not fail on missing python. When python is missing or below required version the configuration will error out. This removes failing silently on python.
This PR will partial solve issue #31476 (strictly the Python issue)
---
Alternative thoughts:
Should CMAKE even test for python ? If you see CMAKE as configuration for the BUILD step and we do not use pytho
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013076501)
@theStack I don't think this matters really, because "number of peers with at least one orphan" is used as a logistically-easier approximation for "number of peers which participate in transaction relay" (total memory usage is allowed to go up with more peers, because it's expected that peers that do transaction relay result in increases memory usage due to that). The fact that this means that peers which don't have any announced remaining orphans are not counted is a side-effect, not the goal.
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013076501)
@theStack I don't think this matters really, because "number of peers with at least one orphan" is used as a logistically-easier approximation for "number of peers which participate in transaction relay" (total memory usage is allowed to go up with more peers, because it's expected that peers that do transaction relay result in increases memory usage due to that). The fact that this means that peers which don't have any announced remaining orphans are not counted is a side-effect, not the goal.
...
💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2172062756)
@yuvicc can you un-resolve this? I'd like other reviewers to see it and yell at me if I'm wrong :)
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2172062756)
@yuvicc can you un-resolve this? I'd like other reviewers to see it and yell at me if I'm wrong :)
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3013114686)
> doing once more shouldn't introduce any noticeable overhead
Looking at `CCoinsViewCache::AccessCoin()`, it calls `FetchCoin`. This either returns a previously cached coin, or fetches it from its base (possibly on disk) and then adds it to the cache.
So the worst case overhead for a _second_ `AccessCoin()` call is having to perform another `try_emplace` on this cache.
The `CCoinsCaching` bench @l0rinc points to tests `AreInputsStandard` using a very small dummy coin cache. That's prob
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3013114686)
> doing once more shouldn't introduce any noticeable overhead
Looking at `CCoinsViewCache::AccessCoin()`, it calls `FetchCoin`. This either returns a previously cached coin, or fetches it from its base (possibly on disk) and then adds it to the cache.
So the worst case overhead for a _second_ `AccessCoin()` call is having to perform another `try_emplace` on this cache.
The `CCoinsCaching` bench @l0rinc points to tests `AreInputsStandard` using a very small dummy coin cache. That's prob
...
👍 instagibbs approved a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-2966618410)
reACK eb330b58d8d20bb4a5002906cb48bb03c5fa1a10
squashes and a couple errant comments deleted
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-2966618410)
reACK eb330b58d8d20bb4a5002906cb48bb03c5fa1a10
squashes and a couple errant comments deleted
🤔 scgbckbone reviewed a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2966639068)
ACK
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2966639068)
ACK
💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2172077826)
For policy no way does this slowdown matter. That said, since we're already doing the computation in both policy and consensus contexts, maybe we can just reuse it? https://github.com/instagibbs/bitcoin/tree/2025-06-patho-bip54
it compiles
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2172077826)
For policy no way does this slowdown matter. That said, since we're already doing the computation in both policy and consensus contexts, maybe we can just reuse it? https://github.com/instagibbs/bitcoin/tree/2025-06-patho-bip54
it compiles
💬 fanquake commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3013155618)
Guix Build:
```bash
f5e794c539bcf8260fe68be5893004f3a8f7aacbaba794704116b0fc38dd5813 guix-build-14653b869b91/output/aarch64-linux-gnu/SHA256SUMS.part
a701102c9ae49f32c780241a0dbfe16589213cbb43d7c48d8ca35de0c9032c2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu-debug.tar.gz
e0f184484a32b5a00c8034a3cb3985e21abf6df342602e6ad4cd3063ccd12f2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu.tar.gz
4ef01b549490fff7
...
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3013155618)
Guix Build:
```bash
f5e794c539bcf8260fe68be5893004f3a8f7aacbaba794704116b0fc38dd5813 guix-build-14653b869b91/output/aarch64-linux-gnu/SHA256SUMS.part
a701102c9ae49f32c780241a0dbfe16589213cbb43d7c48d8ca35de0c9032c2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu-debug.tar.gz
e0f184484a32b5a00c8034a3cb3985e21abf6df342602e6ad4cd3063ccd12f2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu.tar.gz
4ef01b549490fff7
...
💬 leopardracer commented on pull request "correct variable name in p2p_monitor.py":
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3013168737)
> lgtm ACK
@maflcko everything is ok?
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3013168737)
> lgtm ACK
@maflcko everything is ok?
🚀 fanquake merged a pull request: "build: Find Boost in config mode"
(https://github.com/bitcoin/bitcoin/pull/32667)
(https://github.com/bitcoin/bitcoin/pull/32667)
💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3013197158)
Friendly ping @maflcko @theuni @darosior @fanquake who were reviewing https://github.com/bitcoin/bitcoin/pull/28657.
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3013197158)
Friendly ping @maflcko @theuni @darosior @fanquake who were reviewing https://github.com/bitcoin/bitcoin/pull/28657.