💬 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

💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273472102)
I don't think degrading our interface to save a bit of compilation time is an improvement tbh. I didn't look into it in detail but can see moving the sighash types into a separate header being a worthwhile follow-up, though.

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1649764330)
ACK c5fac53bca920626843723869a7677cef639df4d

with the following notes or follow-ups:

- modulo the caveat of https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272918229
- test coverage of the error case(s)
- small release note if user-facing error changes

💬 MarcoFalke commented on pull request "suppressions: note that `type:ClassName::MethodName` should be used":
(https://github.com/bitcoin/bitcoin/pull/28147#issuecomment-1649766365)
lgtm ACK d0c6cc4abe42163aaf081a969d2c449785563ba2

💬 MarcoFalke commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#issuecomment-1649771219)
lgtm ACK bd5ae6c66317de39195ddb38cea3ca05bbd99275