💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273361119)
I think this is on purpose since we use `ignore_incoming_txs` for multiple purposes, see also my suggested diff in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386 (although I now see it can be simplified even further by not passing `ignores_incoming_txs` to the `Options` constructor at all). So, I don't think it's forgotten, but room for improvement in follow-up? Happy to open a pull for this if you think it's an improvement.

💬 fanquake commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#discussion_r1273370949)
Added, but had to use `-m` for macOS users.

💬 MarcoFalke commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273370954)
Yeah, I don't see what is wrong with your diff (apart from it not compiling).

I just don't get the point of adding the `ApplyArgsManOptions` helper when it is not used consistently. Might as well not add it in the first place?

💬 MarcoFalke commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#discussion_r1273372415)
My assumption was to run the `git` command on the machine that is running guix. I don't think macOS can run guix?

💬 MarcoFalke commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1649628432)
> which may reduce build size and speed up build times.

May be good to add numbers?

💬 MarcoFalke commented on pull request "test: Bump walletpassphrase timeout to avoid intermittent issue":
(https://github.com/bitcoin/bitcoin/pull/28089#issuecomment-1649629599)
Thanks, yes.

✅ MarcoFalke closed a pull request: "test: Bump walletpassphrase timeout to avoid intermittent issue"
(https://github.com/bitcoin/bitcoin/pull/28089)

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1649649475)
Thanks so much for taking a look Gleb and for your insight, yeah I have the same concerns. That attack vector is not super trivial, the attacker needs to both learn and then spoof the protected IP range. The release notes could recommend only using ranges for local subnets, or we could enforce that in the code. We could also change it so`forceinbound` does not evict existing peers but *may* add connections beyond the maximum, or set another internal limit just for `forceinbound`

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273422669)
Seems like `std::runtime_error` is still thrown: https://github.com/TheCharlatan/bitcoin/commits/kernelRmUnivalue_8.

👋 dergoegge's pull request is ready for review: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043)

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1649733407)
Rebased and un-drafted, ready for review!

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1273450317)
My thinking was that this should result in more coverage because we'll also cover paths for invalid PoW.

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273454021)
Mmh, we don't do this for any of the other `get_str()` calls, so this might be better for a follow up? Would probably also be good to re-use the text from the type error.

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273460013)
Perhaps move that check back to the callee until the sighash enum is extracted.

💬 MarcoFalke commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#discussion_r1273461027)
But then it shouldn't be included in the git commit. If you run guix in a VM, my thinking was to include the architecture of the VM/guix, not the one of the macOS or BSD machine.

💬 fanquake commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#discussion_r1273462379)
Right. I'm just going to remove this then. As it's impossible to provide generic instructions that work with all our supported signing methods.

📝 fanquake opened a pull request: "suppressions: note that `type:ClassName::MethodName` should be used"
(https://github.com/bitcoin/bitcoin/pull/28147)
Now that the symbolizer is back in play, suppressions can once-again be targeted to functions, rather than file-wide.

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273464151)
Noting that this error doesn't seem to have test coverage, which could be done here before the refactoring to cover the behavior change (or no change), or in a follow-up.

👋 fanquake's pull request is ready for review: "doc: cleanup release process doc"
(https://github.com/bitcoin/bitcoin/pull/28003)

💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273468412)
> Mmh, we don't do this for any of the other get_str() calls

I think we do? E.g. https://github.com/bitcoin/bitcoin/blob/e35fb7bc48d360585b80d0c7f89ac5087c1d405e/src/rpc/util.cpp#L89-L92 and https://github.com/bitcoin/bitcoin/blob/e35fb7bc48d360585b80d0c7f89ac5087c1d405e/src/rpc/util.cpp#L1199