💬 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
...
💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_getaddr_caching.py":
(https://github.com/bitcoin/bitcoin/pull/28144#discussion_r1273665330)
yes, I missed those, now I removed them all!
(https://github.com/bitcoin/bitcoin/pull/28144#discussion_r1273665330)
yes, I missed those, now I removed them all!
💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_getaddr_caching.py":
(https://github.com/bitcoin/bitcoin/pull/28144#discussion_r1273665554)
fixed
(https://github.com/bitcoin/bitcoin/pull/28144#discussion_r1273665554)
fixed
💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_getaddr_caching.py":
(https://github.com/bitcoin/bitcoin/pull/28144#issuecomment-1649991529)
[e4d3787 ](https://github.com/bitcoin/bitcoin/commit/e4d37875e6a440748623250d77ec3be523a1d34d)to 8a20f76: Addressed feedback, thanks!
> Today I learned something new: it is possible to have duplicate (srcaddr, srcport) for TCP connections if the destination is different. Thank you!
I was also surprised by this!
(https://github.com/bitcoin/bitcoin/pull/28144#issuecomment-1649991529)
[e4d3787 ](https://github.com/bitcoin/bitcoin/commit/e4d37875e6a440748623250d77ec3be523a1d34d)to 8a20f76: Addressed feedback, thanks!
> Today I learned something new: it is possible to have duplicate (srcaddr, srcport) for TCP connections if the destination is different. Thank you!
I was also surprised by this!
📝 stickies-v opened a pull request: "Net processing: clamp PeerManager::Options user input"
(https://github.com/bitcoin/bitcoin/pull/28149)
Avoid out-of-bounds user input for `PeerManager::Options` by safely clamping `-maxorphantx` and `-blockreconstructionextratxn`, and avoid platform-specific behaviour by changing `PeerManager::Options::max_extra_txs` from `size_t` to a `uint32_t`. Addresses https://github.com/bitcoin/bitcoin/pull/27499#pullrequestreview-1544114932.
Also documents all `PeerManager::Options` members, addressing https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1272302469.
(https://github.com/bitcoin/bitcoin/pull/28149)
Avoid out-of-bounds user input for `PeerManager::Options` by safely clamping `-maxorphantx` and `-blockreconstructionextratxn`, and avoid platform-specific behaviour by changing `PeerManager::Options::max_extra_txs` from `size_t` to a `uint32_t`. Addresses https://github.com/bitcoin/bitcoin/pull/27499#pullrequestreview-1544114932.
Also documents all `PeerManager::Options` members, addressing https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1272302469.
💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273685388)
Agreed. Addressed in https://github.com/bitcoin/bitcoin/pull/28148.
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273685388)
Agreed. Addressed in https://github.com/bitcoin/bitcoin/pull/28148.
💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273686799)
@glozow's comment re documenting Options members addressed in https://github.com/bitcoin/bitcoin/pull/28149
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273686799)
@glozow's comment re documenting Options members addressed in https://github.com/bitcoin/bitcoin/pull/28149
💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#issuecomment-1650014682)
> Maybe as a followup we could add some pedantic checks in init to make sure they're sane values that fit into 32bits and make `max_orphan_txs` a `uint32_t`.
Addressed in https://github.com/bitcoin/bitcoin/pull/28149
(https://github.com/bitcoin/bitcoin/pull/27499#issuecomment-1650014682)
> Maybe as a followup we could add some pedantic checks in init to make sure they're sane values that fit into 32bits and make `max_orphan_txs` a `uint32_t`.
Addressed in https://github.com/bitcoin/bitcoin/pull/28149
👍 luke-jr approved a pull request: "init: changing -torcontrol help to specify that a default port is used"
(https://github.com/bitcoin/bitcoin/pull/28101#pullrequestreview-1545729648)
utACK
(https://github.com/bitcoin/bitcoin/pull/28101#pullrequestreview-1545729648)
utACK
💬 luke-jr commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1273696968)
nit: I feel like this could be formatted better. Might need a comma after "specified".
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1273696968)
nit: I feel like this could be formatted better. Might need a comma after "specified".
💬 fanquake commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650051389)
Sorry for turning up late to the discussion, but I fail to see how adding the `<script/interpreter.h>`
include into `src/rpc/util.cpp` (the discussion in this thread: https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272918229) would make any difference to compile times, given it's contents is already contained in the preprocessed output of `rpc/libbitcoin_common_a-util.o`?
If you compare the preprocessed output of `rpc/libbitcoin_common_a-util.o` on master (e35fb7bc48d360585b80d0c7
...
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650051389)
Sorry for turning up late to the discussion, but I fail to see how adding the `<script/interpreter.h>`
include into `src/rpc/util.cpp` (the discussion in this thread: https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272918229) would make any difference to compile times, given it's contents is already contained in the preprocessed output of `rpc/libbitcoin_common_a-util.o`?
If you compare the preprocessed output of `rpc/libbitcoin_common_a-util.o` on master (e35fb7bc48d360585b80d0c7
...