💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959969112)
In commit "[bench] TxOrphanage::LimitOrphans"

Would it make sense to introduce this benchmark earlier (and the other ones below), so we can see what effect the previous commit has on it?

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959814286)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"

Use `tx, std::move(announcer_list_pos), ...` to avoid an allocation.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959842788)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"

Would it be possible to do the `list_pos` updating here without needing to return an iteration to push that responsibility to the caller? It would mean `RemoveIterAt` would need to know what peer it's operating in, so that means it's perhaps more appropriate to have it as ` TxOrphanage` member function rather than a `PeerOrphanInfo` member function.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959859482)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"

I think this line, and the 7 lines before it, can be replaced with `RemoveIterAt(it->second.list_pos)`, especially if it can be changed to do the `list_pos` updating internally?

Not very important as the code disappears in the next commit, but would make it more obviously correct.

✅ hebasto closed an issue: "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable""
(https://github.com/bitcoin/bitcoin/issues/31881)

💬 hebasto commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2666041425)
Resolved in https://github.com/bitcoin/bitcoin/pull/31893.

📝 maflcko opened a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896)
`IsArgSet` is problematic:

* It returns whether an arg has been set, even if it has been negated. `IsArgSet` is sometimes used to check for a truthy value, which is wrong, but usually harmless. Cleanup of those cases may or may not be done in a follow-up.
* In most other cases, calling it is redundant, because the immediately following `Get*Arg` calls can already return an `std::optional` nullopt value to indicate an unset arg.

So relieve both issues by removing all `IsArgSet` that are re
...

💬 maflcko commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959977366)
nit: I find the `!Value(gArgs).isNull()` with the double negation and verbosity over just `Get(gArgs)` a bit ugly and I think it would be better to remove `IsArgSet` as much as possible. In most cases it is not needed anyway, or used incorrectly. About this one, I left a comment here: https://github.com/bitcoin/bitcoin/pull/31887/files#r1959923000

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959978900)
Agree on double-quotes in more places giving nicer output, but would not rephrase or remove legacy wallets at this point.
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the \"wallets\" directory), or a legacy wallet's .dat file (within the \"wallets\" directory). The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```
I
...

📝 Sjors opened a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897)
For the coinbase `vTxFees` used a dummy value of -nFees.

Similarly the first `vTxSigOpsCost` entry was calculated from
the dummy coinbase transaction.

This was introduced in #2115, but the values were never returned by the RPC or used in a test.

Set both to 0 instead and add code comments to prevent confusion.

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960001093)
I think invalidating ACKs is more a consideration for critical or difficult-to-review changes.

If loading legacy wallets will be disabled in the next release, it may be worth not adding a mention here that would require attention/removal.

🚀 fanquake merged a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892)

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960003712)
I opened #31897 to drop this `-nFees` dummy.

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960005999)
> Agree on double-quotes in more places giving nicer output

The suggested diff doesn't only add double quotes but also proposes a rewrite for clarity between wallet directory and wallets directory.

✅ prusnak closed an issue: "rpc: dumptxoutset as sqlite file"
(https://github.com/bitcoin/bitcoin/issues/24628)

💬 prusnak commented on issue "rpc: dumptxoutset as sqlite file":
(https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-2666086329)
> I think this is resolved with the merged of [#27432](https://github.com/bitcoin/bitcoin/pull/27432)?

Yes

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1960007754)
Changed from -1 to 0, because if someone ignores the repeated instruction to not use this value, at least they won't incorrectly sum up `vTxFees`.

💬 rkrux commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2666108177)
One way to ensure code structure correctness and get rid of the brittleness noticed here (https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959636120) is to incorporate the following change. In a cursory attempt, I have this that has minimal diff that passes the tests. LMK how does this sound.

```cpp
void CheckUnparsable(const std::string& prv, const std::string& pub, const std::string& expected_error, bool check_prv_error_only = false)
{
FlatSigningProvider keys_priv, keys_pu
...

🤔 hodlinator reviewed a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2624027023)
Code reviewed 3252f3bab02605129a568112f650ef3cb29703b7

ACK 9184ec9e8ea6efd4d0d3625a06f223156f940243

Advise dropping 3rd commit, the others seem good.

💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960004078)
Weird to change inheritance from `class -> class` to `struct -> class`.