💬 stratospher commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#issuecomment-1584966952)
@theStack I've updated the PR to include your suggestion. Documenting it like this might also help people who haven't gone through the algo notice this behaviour in the tests.

📝 Xekyo opened a pull request: "[coinselection] Increase SRD target by change_fee"
(https://github.com/bitcoin/bitcoin/pull/27846)
I discovered via fuzzing of another coin selection approach that at extremely high feerates SRD may find input sets that lead to transactions without change outputs. This is an unintended outcome since SRD is meant to always produce a transaction with a change output—we use other algorithms to specifically search for changeless solutions.

The issue occures when the flat allowance of 50,000 ṩ for change is insufficient to pay for the creation of a change output with a non-dust amount, at and a
...

💬 Xekyo commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1224612164)
I think you might get some funny behavior here, if `min_viable_change` is smaller than `m_change_fee` as mentioned above, also see https://github.com/bitcoin/bitcoin/pull/27846

👍 ryanofsky approved a pull request: "p2p: skip netgroup diversity follow-up"
(https://github.com/bitcoin/bitcoin/pull/27467#pullrequestreview-1472859378)
Code review ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd

This change seems like an improvement to me, but I also think the early comment calling it "small and trivial" was appropriate and useful for clarifying the purpose of the change. Reviewers need to be able to express their honest opinions so limited review bandwidth goes to the right places, and so PR authors have a chance to improve and justify their PRs and not sit around wondering why a PR isn't being reviewed

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