👋 dergoegge's pull request is ready for review: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043)
(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!
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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)
(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
(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.
(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
(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
(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
(https://github.com/bitcoin/bitcoin/pull/28003#issuecomment-1649771219)
lgtm ACK bd5ae6c66317de39195ddb38cea3ca05bbd99275
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273490496)
Ah right, I just looked at the wrong examples :P - will push a patch for this.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273490496)
Ah right, I just looked at the wrong examples :P - will push a patch for this.
💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273494697)
It took a long time to compile bitcoind on my 2-core cpu until I upgraded last year, so given this particular tradeoff of the check in caller/callee, I like faster builds :)
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273494697)
It took a long time to compile bitcoind on my 2-core cpu until I upgraded last year, so given this particular tradeoff of the check in caller/callee, I like faster builds :)
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273495300)
> I think we do? E.g.
I think all of that code should be removed and the checks be done by RPCHelpMan.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273495300)
> I think we do? E.g.
I think all of that code should be removed and the checks be done by RPCHelpMan.
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273498300)
> It took a long time to compile bitcoind on my 2-core cpu until I upgraded last year, so given this particular tradeoff of the check in caller/callee, I like faster builds :)
I don't think removing a few lines from a single cpp file is going to change that, unless there are number available.
Generally, the most expensive headers are the serialize one and the boost ones. And serialize.h will still be included here, because of `CScript`.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273498300)
> It took a long time to compile bitcoind on my 2-core cpu until I upgraded last year, so given this particular tradeoff of the check in caller/callee, I like faster builds :)
I don't think removing a few lines from a single cpp file is going to change that, unless there are number available.
Generally, the most expensive headers are the serialize one and the boost ones. And serialize.h will still be included here, because of `CScript`.
💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273507748)
> I don't think removing a few lines from a single cpp file is going to change that, unless there are benchmarks available.
`script/interpreter.h` is ~350 lines ATM, with some template classes, so my point is that if we don't have a strong need to include it, seems best not to.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273507748)
> I don't think removing a few lines from a single cpp file is going to change that, unless there are benchmarks available.
`script/interpreter.h` is ~350 lines ATM, with some template classes, so my point is that if we don't have a strong need to include it, seems best not to.