💬 Sjors commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368574744)
Demo branch `MULTIPROCESS=1` and without `x86_64-w64-mingw32`:

```
x86_64
e31ab0eeff88048301f113731a9b829df63e7db2c6a83c3a54e14b6f423aa4a8 guix-build-d8ec933456bc/output/aarch64-linux-gnu/SHA256SUMS.part
77f4a9481b4ce7df26549c6001384e066a1de3cb1b81ba950c29cc41b0b5c058 guix-build-d8ec933456bc/output/aarch64-linux-gnu/bitcoin-d8ec933456bc-aarch64-linux-gnu-debug.tar.gz
ee471d630ea0eb71c2040a3b9e408c4f1709ee29d2da4ecbc1931d93b67e5495 guix-build-d8ec933456bc/output/aarch64-linux-gnu/bitcoi
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2368602184)
Thanks for @hodlinator and @maflcko for the comments.I'll go ahead and update this PR in case marco is interested in looking at it since dropping support for flag combinations (https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2256044988).

In the meantime though I'm slowly working on the branch in #22978. Compared to this PR, that approach will be a larger change. This PR adds only ~100 lines if you exclude tests and documentation by only adding flags while not change existing c
...

💬 willcl-ark commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2368602687)
ACK 42edb2f4a5900e0b2451a7446d58962c0bf0095d

I agree with @l0rinc that this seems conceptually nicer, and also see a much smaller time diff since 30888:

```log
# master @ 33adc7521cc8bb24b941d959022b084002ba7c60
________________________________________________________
Executed in 181.29 secs fish external
usr time 38.26 mins 214.00 micros 38.26 mins
sys time 1.49 mins 55.00 micros 1.49 mins


# this PR rebased on master @ 33adc7521cc8bb24b941d959022
...

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368636422)
> I don't understand why it is switching from _DIR variables to _ROOT variables. According to https://chatgpt.com/c/66f16a25-c068-800a-bba6-36d030d42869 both approaches seem roughly equivalent, but I don't understand if there was another reason to switch from _DIR to _ROOT.

1. https://cmake.org/cmake/help/latest/policy/CMP0074.html
2. `<PackageName>_ROOT` has top priority for the [`find_package()`](https://cmake.org/cmake/help/latest/command/find_package.html) command.
3. `<PackageName>_ROO
...

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771685550)
fixed

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771685757)
taken

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2368661926)
> Commit https://github.com/bitcoin/bitcoin/commit/19bc71caf6a43702946f1bc15696fe14afa24fda description says "delay" when it should say "check_interval".

Also fixed.

💬 ryanofsky commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368665229)
Thanks for the clarifications. I agree that issues (2) (3) and (4) are not directly related to this PR and would be best to address separately if it makes sense to follow up on them. Those were just things that I didn't understand, and seemed to intersect with the issue.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2368720193)
I have replaced the `clusterlin_add_dependency` and `clusterlin_cluster_serialization` tests with a new `clusterlin_depgraph_sim` test that performs an arbitrary sequence of `AddTransaction`, `AddDependencies`, and `RemoveTransactions` calls, and compares the final result with a simulated version. With that, `Cluster` was easy to delete.

🤔 ryanofsky reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2322682262)
Rebased 41bdf3d025f900a59ec14d5b497a31a2d84eea52 -> 87b6d4cb5a5c04f6c8542c633c3bfa5f76901d43 ([`pr/argcheck.40`](https://github.com/ryanofsky/bitcoin/commits/pr/argcheck.40) -> [`pr/argcheck.41`](https://github.com/ryanofsky/bitcoin/commits/pr/argcheck.41), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/argcheck.40-rebase..pr/argcheck.41)) due to conflicts with #29043 and #30618

Followup adding support for argument combinations is commit c919f51c044f0e80ecd301f9c9d396ddff2331ba ([b
...

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