🤔 l0rinc requested changes to a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2965729639)
Concept ACK

Regardless of whether the soft-fork will be accepted, it's important to guard against this early.

However, the current implementation introduces a significant slowdown and I think we could make the testing more solid by explicitly validating that we didn't soft-fork yet.
Therefore it's an Approach NACK from me, please see the detailed explanation below.

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171446494)
Seems wasteful to iterate and parse `tx.vin[i].scriptSig` twice, we should already know the last item that the `scriptSig` pushed onto the stack after the first iteration - can we optimize that?

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171603715)
similarly to other `emplace_back` calls here, we don't need the explicit constructor (it avoids constructing a temporary):
```suggestion
tx_max_sigops.vin.emplace_back(prev_txid, i);
```

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171450864)
Note: since this magic value is referencing a soft-fork that may or may not be applied, we could mention it here for context: https://github.com/bitcoin/bips/blob/master/bip-0054.md#specification

Q: How often do we expect mined blocks to contradict this new policy rule?
Asking because of https://b10c.me/observations/11-invalid-blocks-783426-and-784121, claiming:
> On April 1st, 2023, F2Pool mined an invalid block at height 783426. Bitcoin Core nodes rejected the block with the reason bad-bl
...

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171492254)
nit: multiple typos in commit message 27e54beff7d1c9ac68bee379bb6d971a775b9841

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171470960)
Similarly to the discussion on https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095472966, it seems to me we could fail early by exiting if the first one is already too big:
```suggestion
sigops += tx.vin[i].scriptSig.GetSigOpCount(/*fAccurate=*/true);
if (sigops > MAX_TX_LEGACY_SIGOPS) {
return false;
}
```
or inversely, if this isn't performance critical, we can do the check at the very end instead, to speed up the happy path by not doing in
...

💬 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
...

💬 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
...

💬 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
...

💬 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);
```

💬 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".

💬 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?

💬 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

💬 purpleKarrot commented on pull request "build: Find Boost in config mode":
(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
...

🤔 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
...

💬 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.
...

💬 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 :)

💬 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
...

👍 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