💬 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.
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273518096)
> seems best not to.
Again, I am saying it would be good to have benchmarks. If stuff is split too much, it can easily lead to longer build times, because the compiler needs to be invoked more often. (Maybe not in this case, if the new module is header-only)
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273518096)
> seems best not to.
Again, I am saying it would be good to have benchmarks. If stuff is split too much, it can easily lead to longer build times, because the compiler needs to be invoked more often. (Maybe not in this case, if the new module is header-only)
💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273525152)
Agree. In the absence of benchmarks, I'd consider keeping the check in the callee, then move it to the caller if the new module is done and is as lightweight as it looks it might be.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273525152)
Agree. In the absence of benchmarks, I'd consider keeping the check in the callee, then move it to the caller if the new module is done and is as lightweight as it looks it might be.
📝 stickies-v opened a pull request: "Refactor: consistently use ApplyArgsManOptions for PeerManager::Options"
(https://github.com/bitcoin/bitcoin/pull/28148)
Consistently use `ApplyArgsManOptions` for `PeerManager::Options`, and initialize `PeerManager::Options` early to avoid reading `"-blocksonly"` twice. Suggested in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386 and also requested in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273346189.
No behaviour change, but the `TestingSetup` is now also able to access `"-blocksonly"`.
(https://github.com/bitcoin/bitcoin/pull/28148)
Consistently use `ApplyArgsManOptions` for `PeerManager::Options`, and initialize `PeerManager::Options` early to avoid reading `"-blocksonly"` twice. Suggested in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386 and also requested in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273346189.
No behaviour change, but the `TestingSetup` is now also able to access `"-blocksonly"`.
💬 Crypt-iQ commented on issue "error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed":
(https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1649931679)
I was able to reproduce this by adding some sleeps:
<details>
<summary>diff on bc4f6b13feb29146b7e10e86f93dc7f6fb6937f2 </summary>
<br>
```diff
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index d344c8bfb..bb645ae94 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -17,6 +17,9 @@
#include <unordered_map>
#include <utility>
+#include <chrono>
+#include <thread>
+
std::string RemovalReasonToString(const MemPoolRemovalRe
...
(https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1649931679)
I was able to reproduce this by adding some sleeps:
<details>
<summary>diff on bc4f6b13feb29146b7e10e86f93dc7f6fb6937f2 </summary>
<br>
```diff
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index d344c8bfb..bb645ae94 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -17,6 +17,9 @@
#include <unordered_map>
#include <utility>
+#include <chrono>
+#include <thread>
+
std::string RemovalReasonToString(const MemPoolRemovalRe
...
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1649953642)
Updated c5fac53bca920626843723869a7677cef639df4d -> d87edf309ffe60ae044a57ce9a0d9cc711edc365 ([kernelRmUnivalue_7](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_7) -> [kernelRmUnivalue_8](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_7..kernelRmUnivalue_8))
* Addressed @jonatack's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272918229), introduced `sighashtyp
...
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1649953642)
Updated c5fac53bca920626843723869a7677cef639df4d -> d87edf309ffe60ae044a57ce9a0d9cc711edc365 ([kernelRmUnivalue_7](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_7) -> [kernelRmUnivalue_8](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_7..kernelRmUnivalue_8))
* Addressed @jonatack's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272918229), introduced `sighashtyp
...