💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171711143)
Thanks for incorporating the feedback! These should be snake_case:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L392-L393
as per:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/developer-notes.md?plain=1#L89-L90

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171692864)
Still doing it in these places:
https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L298
https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L1110

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171513363)
Still seeing it in 83b6253eb3921a79231f450bb43e1977ea835b5e.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171528297)
Re ceiling - Ah, should have looked the symbols up. I'm fine either way, but would guess this is clearer for most programmers.

---

Sorry, didn't mean it so literally, maybe something like:
```diff
- Note: this is somewhat a retake of #27591
+ Note: this picks up work from the closed #27591.
```
I would put that together with the "Fixes #32775" at the bottom of the description, up to you.

---

You're still using `sigop count` in the formula where I think `sigop cost` would be more
...

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171769104)
Q1. Yeah, I think the interface is less confusing if we remove the condition. #27591 seems to use the term "sigop-adjusted size" rather than "sigop-equivalent size". So could change the description from:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L284
to:

"Sigop-adjusted size in bytes, only present for mempool transactions."

(Unless we implement Q2, which I agree is not critical).

---

Maybe replace "sigop-equivalent"
...

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171533354)
Nice!

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171892100)
I think you are right; I will generalise it to any string args which might contain `=`

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3012839324)
Concept ACK 14e2125ee0297f0cb046ca91b6ead20052f24678. Thanks for the fix!

If possible it would be good to add python tests to check the new behavior and I also agree with [comment](https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171811426) that this change doesn't need to be limited to base64 parameters and could probably be extended to other string parameters. But maybe the "If parameter is unknown for a given known string method, then treat the whole string as positional" behavi
...

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