🚀 ryanofsky merged a pull request: "p2p: skip netgroup diversity follow-up"
(https://github.com/bitcoin/bitcoin/pull/27467)

💬 stratospher commented on pull request "test: Remove modinv python util helper function":
(https://github.com/bitcoin/bitcoin/pull/27538#issuecomment-1584977489)
we'd need to remove util from `TEST_FRAMEWORK_MODULES` in test_runner too. i've included this in #24005 where anyways `TEST_FRAMEWORK_MODULES` has to be [touched](https://github.com/bitcoin/bitcoin/pull/24005/commits/a250d70e856f2178e70f0391bf469fdcdd3c3006).

👍 ryanofsky approved a pull request: "kernel: Remove args, settings, chainparams, chainparamsbase from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27576#pullrequestreview-1472898526)
Code review ACK db77f87c6365cb5f414036d6bfb1a12705772028. Looks great! I left a suggestion to clean up a unit test in a possible followup, but I will just go ahead and merge this now if tests pass locally.

💬 ryanofsky commented on pull request "kernel: Remove args, settings, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#discussion_r1224638890)
In commit "refactor: Add path argument to FindSnapshotChainstateDir" (8789b11114b4bd6c7ee727dffbc75a6bdf20dd27)

Throughout this file, it would make tests clearer and reliant on details of test setup if the new
`m_args.GetDataDirNet()` calls were replaced with `chainman.m_options.datadir`

👍 ryanofsky approved a pull request: "kernel: Remove args, settings, chainparams, chainparamsbase from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27576#pullrequestreview-1472909551)
In commit "scripted-diff: move settings to common namespace" (db77f87c6365cb5f414036d6bfb1a12705772028)

Not important but I noticed the scripted diff is escaping space and colon characters. I think there is not actually a need for all those backslashes, and they don't do anything

🚀 ryanofsky merged a pull request: "kernel: Remove args, settings, chainparams, chainparamsbase from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27576)

💬 ryanofsky commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1585023314)
The code change here seems very simple, but since this PR does add a new RPC it would be useful to have more concept ACKs to know whether this is appropriate to merge. I noticed a number of contributors added github reactions to some comments here, so perhaps more of the people following the discussion could add concept acks or mention any reservations

💬 hebasto commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#issuecomment-1585027163)
> Silent merge conflict (with `-werror`)

Rebased. Sorry for not addressing comments promptly.

> Following patch fixes the silent merge conflict for me after a rebase:

@ryanofsky Thanks! Applied.

💬 hebasto commented on issue "Can't start bitcoin-qt by double-click on Debian 11":
(https://github.com/bitcoin-core/gui/issues/730#issuecomment-1585034303)
We've discussed this with @kallerosenbaum offline and decided to close this issue as a system-specific one and having the workaround pointed out in https://github.com/bitcoin/bitcoin/issues/27655#issue-1708983426.

✅ hebasto closed an issue: "Can't start bitcoin-qt by double-click on Debian 11"
(https://github.com/bitcoin-core/gui/issues/730)

💬 Xekyo commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1224677456)
I added the assert suggested by @glozow

💬 Xekyo commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#issuecomment-1585035235)
Added the additional assert suggested above in https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1221909729

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1224732592)
I think that is a misunderstanding caused by the vector fields being number 0–7 and the txs being numbered 1–8. I’ve got a draft where I change the names of the transactions to start with number 0.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1224732834)
Great idea, will do.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1224733097)
I am removing the unused includes.

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224765526)
> I also think it might be clearer just to delete the BitcoinApplication::getExitStatus method entirely and just return node().getExitStatus() at the end of GuiMain()

That was my first idea too but `GuiMain()` is a static function with no direct access to the node interface. The interface is created inside the `BitcoinApplication::createNode` class method.

> This is ok for now, but if you want to future-proof the code and make it compatible with https://github.com/bitcoin/bitcoin/pull/1010
...

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224766573)
done, deleted

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224767267)
done. Refactored to pass `exit_status` ref.

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224767427)
done

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224770367)
yeah, same note applies for the other loop that was moved out of the try catch. The one that calls `IsSwitchChar()`.