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

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273736467)
Ugh, how did I miss those, will re-push.

📝 MarcoFalke opened a pull request: "test: Avoid intermittent issues due to async events in validationinterface_tests"
(https://github.com/bitcoin/bitcoin/pull/28150)
Currently the tests have many issues:

* They setup the genesis block, even though it is not needed
* They queue an async `UpdatedBlockTip` even, which causes intermittent issues: https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1650064645

Fix all issues by trimming down the setup to just `ChainTestingSetup`.

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1273739178)
> I wonder what this failure looks like in practice, though. Could we write a test for it?

I could not come up with a way to test this with our existing code. If anybody has any pointers for how to achieve this, I'd happy to give it a shot at implementing the test.

> Can you describe what they should be doing, and do you know if they actually are doing it?

Callers of `FlushStateToDisk` already need to handle failures of the block index writing, so I don't think they require extra logic
...

💬 MarcoFalke commented on issue "error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed":
(https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1650073657)
Let's continue discussion in https://github.com/bitcoin/bitcoin/pull/28150

✅ MarcoFalke closed an issue: "error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed"
(https://github.com/bitcoin/bitcoin/issues/28146)

💬 MarcoFalke commented on pull request "test: Avoid intermittent issues due to async events in validationinterface_tests":
(https://github.com/bitcoin/bitcoin/pull/28150#issuecomment-1650075726)
An alternative might be to call `SyncWithValidationInterfaceQueue` at the start of the test.