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

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1650081711)
Looks like CI is now green :)

Also the test failure from https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1521387810 can now be tested on this pull with just:

```
MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh
```

Output:
```
test 2023-07-25T15:37:42.430000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
F
...

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650091313)
Updated d87edf309ffe60ae044a57ce9a0d9cc711edc365 -> 6960c81cbfa6208d4098353e53b313e13a21cb49 ([kernelRmUnivalue_8](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_8) -> [kernelRmUnivalue_9](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_8..kernelRmUnivalue_9))

* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273714341), fixing release note.
...

💬 MarcoFalke commented on pull request "test: Drop 22.x node from TxindexCompatibilityTest":
(https://github.com/bitcoin/bitcoin/pull/28070#issuecomment-1650131175)
Dropped that change for now, to save it for a later pull.

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

Tested it and both approaches work, not sure which is better. Using `ChainTestingSetup` should be faster?