💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601391240)
nit: perhaps move semantics would make sense here?
```suggestion
node.mempool = std::make_unique<CTxMemPool>(std::move(mempool_opts), mempool_error);
if (!mempool_error.empty()) {
return InitError(mempool_error);
}
LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), node.mempool->m_opts.max_size_bytes * (1.0 / 1024 / 1024));
```

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601457942)
nit: this approach has the potential risk that if additional error handling is added to the CTxMemPool ctor, the fuzzer would silently ignore it if this test is not also updated. Could avoid that by specifically silencing only this error?

<details>
<summary>git diff on dacdb7962c</summary>

```diff
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index eefa566d31..1a95ff4ab6 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@
...

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601479946)
Not sure about this if it gets moved again when the for loop iterates.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2112341321)
Thanks!

On Windows (Vista and higher, which is all we care about) getting the default gateway is straightforward, there's `GetBestRoute2` that does all the work. It's part of netioapi which we already need for interface enumeration. MacOS's sysctl is a bit uglier.

> If PCP servers announced themselves via DNS-Based Service Discovery [rfc6763](https://www.rfc-editor.org/rfc/rfc6763.html) that would also make things easier.

i don't think it really would, we'd need some special library for
...

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2112349111)
rebased due to merge conflict

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2112366413)
rebased due to conflict

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112374433)
Re https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2057592650

> nit: some IWYU fixes, mostly because of the newly introduced bilingual_str and Assert usage:

A bunch of these were broken before this PR, so I wasn't sure if they make sense to repair here. For test files I stopped asking for repairing them when reviewing other PRs, it just feels too Sisyphean if they are not consistently checked by other reviewers anyway. I wish we'd start enforcing this properly though.

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601574911)
nit: `JSONErrorReply` -> `JSONRPCErrorReply`, although it could be argued that it actually does write a JSON object in the response.

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112426141)
Thank you for the review @stickies-v,

Updated dacdb7962c5fef8db26f6fa31facb606165d1d1e -> 27af3910e5fa9915d4913b82443db216d1b1d61b ([mempoolArgs_7](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_7) -> [mempoolArgs_8](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_7..mempoolArgs_8))

* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601457942), checking s
...

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2112430969)
reACK 186a00e644 via range-diff

💬 glozow commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#issuecomment-2112437062)
reACK 9365baa489

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601589963)
Oops no you're absolutely right, I didn't see the for-loop.

💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1601592294)
Not sure I understand this change either. This seems to be mixing the third alternative in the pull request description with the approach you wanted to take?

> Defeats the purpose of a RAII wrapper for FILE* which automatically closes the file when it goes out of scope and there are a lot of users of AutoFile.

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112454690)
re-ACK 27af3910e5fa9915d4913b82443db216d1b1d61b

👍 cbergqvist approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2057910491)
re ACK cbc6c440e3811d342fa570713702900b3e3e75b9

All functional tests passed (with a few automatic skips), except `feature_dbcrash` - slow, unrelated => excluded, and `feature_index_prune` => timed out because rebase with bumped timeout has been held-off.

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601581136)
Would have gone the opposite way and called it `throw_errors` since it is an.. exception.. to maintain legacy behavior. Sorry for not catching that earlier.

💬 sr-gi commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1601615548)
Oh my bad, I was thinking about children that may have multiple missing parents, not only the conflicting one.

💬 sr-gi commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1601615800)
Yep, my bad, nvm

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1601658971)
Covered in [55f16f5](https://github.com/bitcoin/bitcoin/pull/30065/commits/55f16f56f4abeae1f5560cc616ea0b9b31a88073)

💬 t-bast commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2112590380)
While I wouldn't trust myself to correctly code-review this PR, I'd be happy to work on e2e tests that would leverage this for lightning channels fee-bumping (based on [eclair](https://github.com/acinq/eclair)) if it can help validate the logic and get this PR merged.

I'd like to highlight again how important this feature is for lightning (and probably for many other L2 protocols on top of bitcoin today). This is the critical step that allows us to mitigate pinning of a commitment transaction
...