💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771696767)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1767576075

> nit: Remove double spaces

Thanks! Applied suggestion

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771700848)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771310361

> Feels like this is encouraging `BOOL | STRING` usage and should be broken out too if we are postponing that support.

Good catch. Makes sense to drop the example since it doesn't work yet.

💬 tdb3 commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#issuecomment-2368756862)
> Thanks! I've cherry-picked your commit, but reordered it to appear before the test introduction commit and changed the commit subject accordingly (introduce constant, instead of deduplicating). Let me know if that's okay for you.

Looks great, thanks.

👍 tdb3 approved a pull request: "test: add test for specifying custom pidfile via `-pid`"
(https://github.com/bitcoin/bitcoin/pull/30724#pullrequestreview-2322758022)
ACK 04e4d52420a0e6bf40d4bd6fe1f31f66db9eab0a

👍 itornaza approved a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2322806774)
re ACK d59f5a31a1b888051eb47e2f8a5fb8d901de1d10

🤔 mzumsande reviewed a pull request: "test: Add missing sync_mempools() to fill_mempool()"
(https://github.com/bitcoin/bitcoin/pull/30948#pullrequestreview-2322840339)
Concept ACK, one question

💬 mzumsande commented on pull request "test: Add missing sync_mempools() to fill_mempool()":
(https://github.com/bitcoin/bitcoin/pull/30948#discussion_r1771793625)
This works, but it slows the test down a lot (runtime goes down from 17s to 1m 37s for me).

I think we actually only care that at least one tx gets evicted from the mempool of each node, so did you consider having only one sync call after the first large tx is generated (plus one at the end)? Note that it's probably not enough to have the sync already after `tx_to_be_evicted_id` because that tx is so small that it could coexist with the final set of transactions (although I guess we could mak
...

💬 mzumsande commented on issue "Intermittent failure in p2p_1p1c_network.py", line 58, in raise_network_minfee assert_greater_than(node.getmempoolinfo()['mempoolminfee'], FEERATE_1SAT_VB) ; AssertionError: 0.00001000 <= 0.00001000":
(https://github.com/bitcoin/bitcoin/issues/30922#issuecomment-2368864343)
Of course, if I'd been planning on opening a fix myself I would have mentioned that. I wasn't sure how to fix this best, see also my comment in the PR.

📝 achow101 opened a pull request: "test: Use shell builtins in run_command test case"
(https://github.com/bitcoin/bitcoin/pull/30952)
Uses the [suggested command](https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363906135)

Fixes #30938

💬 achow101 commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2368874022)
> The cleanest fix would probably be https://github.com/bitcoin/bitcoin/pull/29868/files#diff-4ea52ba9a5642d6946fb0016ca168b6647a8c6403d9e15e2a7197e72022824d4R87 `const std::string command{"sh -c 'echo err 1>&2 && false'"};`.

Done in #30952

> I'd suggest reverting [199bb09](https://github.com/bitcoin/bitcoin/commit/199bb09d88e28d951c5068eb65643390dbedd066), as workarounds for #30601 and #30608 are available.

If @maflcko's suggestion is good enough, I'd prefer to use that rather than re
...

🤔 furszy reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2322913465)
ACK 5bcc578

📝 itornaza opened a pull request: "refactor: Appropriate re-naming of MAX_OPCODE after tapscript"
(https://github.com/bitcoin/bitcoin/pull/30953)
In `src/script/script.h:216` we define `MAX_OPCODE` to denote the maximum opcode that can be used in script.

However, after the addition of the `OP_CHECKSIGADD` with taproot in the opcode set, the `MAX_OPCODE` naming looks somehow misleading.

Renaming the variable to `MAX_SCRIPT_OPCODE` clearly denotes that the variable is intended for and referring exclusively to script (and not tapscript).

💬 achow101 commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209)
In 00a82f45004a887c2b58ed8da3db9280e0ec8b5c "multiprocess: Add serialization code for BlockValidationState"

There are some places where a `BlockValidationState` is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a `BlockValidateState` were serialized then deserialized, these assertions would be hit. I don't know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble
...

💬 achow101 commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747)
In 246fd7faae85e871ba78101f1dee8d795437b8f6 "multiprocess: Add serialization code for vector<char>"

Why not introduce Span constructors for those types instead of this field builder if that would simplify things?

💬 achow101 commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488)
In 246fd7faae85e871ba78101f1dee8d795437b8f6 "multiprocess: Add serialization code for vector<char>"

This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?

💬 fjahr commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1771952837)
And aside from only hitting this in the context of rescan + blocks missing this is just about showing the progress and not the actual functionality the user seeks. So it didn't feel important enough for me to use another category mainly because of that. Unless there are strong opinions against it I will keep this as is.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2369170805)
> You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)

I don’t deny it that you need only one low-latency connection to run a pretty effective tx spamming attack. Though, in my understanding, you’re poi
...

💬 achow101 commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2369179390)
ACK 7942951e3fcc58f7db0059546d03be9ea243f1db

🚀 achow101 merged a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409)

💬 achow101 commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2369211315)
ACK f20fe33e94c6752e5d2ed92511c0bf51a10716ee