💬 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.

💬 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)

💬 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.

📝 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"`.

💬 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
...

💬 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
...

💬 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!

💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_getaddr_caching.py":
(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!

📝 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.

💬 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.

💬 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

💬 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

👍 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

💬 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".

💬 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
...

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1650056185)
Thank you for the excellent review @ryanofsky.

Updated d1c92b57a72b62ffa202f1d3d357b59befdc9c12 -> f7072be104a8cf0516def7a4d9fca4791a8d2269 ([kernelReturnOnAbort_1](https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort_1) -> [kernelReturnOnAbort_2](https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelReturnOnAbort_1..kernelReturnOnAbort_2))

* Split the changes up into three commits
* Addressed @rya
...

👍 stickies-v approved a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1545751164)
ACK d87edf309ffe60ae044a57ce9a0d9cc711edc365

Removes dependency on UniValue in kernel, and imo improves the interface of dealing with sighashtype user input along the way.

No strong view on keeping/reverting the separate `script/sighashtype.h` header, happy to go either way. Unnecessary code churn is always better to be avoided, but I'd also rather not bike-shed over it. Will quickly re-ACK if consensus is to revert it, though.

💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273714341)
nit: `signrawtransactionwithwallet` and `walletprocesspsbt` also affected

💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273709151)
nit: missing `#include <script/sighashtype.h>`?

💬 MarcoFalke commented on issue "error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed":
(https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1650064645)
> UpdatedBlockTip runs in the scheduler thread

Thanks. That sounds like a bug. The unit test should be single threaded, or alternatively drop all async notifications before starting.