:lock: fanquake locked an issue: "Github"
(https://github.com/bitcoin/bitcoin/issues/27568)

💬 josibake commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1184671009)
The release notes could be a bit more detailed, maybe mentioning that this aligns `DisconnectNode` and the `disconnectnode` rpc. It's nice to have at least a summary of the motivation in the release notes.

🤔 josibake reviewed a pull request: "rpc, p2p: allow `disconnectnode` with subnet"
(https://github.com/bitcoin/bitcoin/pull/26576#pullrequestreview-1412513149)
crACK https://github.com/bitcoin/bitcoin/pull/26576/commits/23f4c2cb452d25f61dada898d5cc4c74f72e0145

💬 josibake commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1184667474)
it would be great to add an example for subnets in the help section below.

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184686191)
Seems like it's a bug: https://github.com/include-what-you-use/include-what-you-use/issues/648

Since it sounds like it's going to be fixed in an upcoming version, I'll remove the `memory` include.

💬 willcl-ark commented on issue "Coinstats index corrupted after invalidateblock and clean shutdown":
(https://github.com/bitcoin/bitcoin/issues/27558#issuecomment-1534309409)
Thanks @mzumsande, I can confirm that that patch does fix the described issue for me too.

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184783154)
Looks like this fix is part of the branch we are using, so this seems a separate bug?

💬 dergoegge commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1184786408)
Afaict this won't work here, because the thread might not finish and then the pool would just get clogged (see comment about abandoning the thread).

Performance should also not really matter here since this doesn't happen that often and startup time for a thread vs. time of a DNS lookup is probably negligible.

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184822728)
Huh, yeah, seems like it. But I guess the issue is similar to what is happening here?

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184831690)
I'm not sure if it is possible for lambda's to shadow a function that they call. Can you show me what you mean?

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184832621)
I guess it would help upstream if someone created an issue with a minimal reproducer and minimal steps to reproduce, starting from scratch.

📝 MarcoFalke opened a pull request: "refactor: Remove need to pass chainparams from BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/27570)
Seems confusing to pass chainparams to each method individually, when the params can't change anyway for the whole lifetime of the block manager, and also must be equal to the ones used by the chainstate manager.

Fix this issue by removing them from the methods and instead storing a reference once in a member field.

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184846753)
Apologies for sniping this in #27570. Feel free to NACK

💬 hebasto commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1534536200)
Concept ACK.

📝 MarcoFalke opened a pull request: "ci: Run iwyu on all src files"
(https://github.com/bitcoin/bitcoin/pull/27571)
This makes it easier to look at the CI output of a file without having to manually add it first to the list.

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1534586459)
Thank you for the review and discussion @MarcoFalke!

Updated f59f0c91acb7a35b767bb0dc75ed8b10add81d9f -> db668b3644883c07064b46b4e2cfd269ac9dffbd ([removeBlockstorageArgs_17](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_17) -> [removeBlockstorageArgs_18](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_18), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_17..removeBlockstorageArgs_18))
* Reverted a previous change mo
...

📝 theStack opened a pull request: "test: dedup file hashing using `sha256sum_file` helper"
(https://github.com/bitcoin/bitcoin/pull/27572)
Rather than doing the open/read/hash-steps manually in the affected functional tests, we can just use the `sha256sum_file` helper from the utils module instead.

Note that for the tool_wallet.py test, the used hash is changed from sha1 to sha256, but as the only purpose is to detect file content changes, this doesn't matter. Also, the optimization using `memoryview` is overkill here, as the opened file has only a size of 24KiB and determining the hash via the helper doesn't take longer than a
...

💬 fanquake commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1534696982)
cc @theStack @MarcoFalke

🤔 dergoegge reviewed a pull request: "fuzz: BIP 42, BIP 30, CVE-2018-17144"
(https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1410594057)
Tested ACK fa18fe3976a0f99480ce42dc0c1df7143967bf4d

I reintroduced CVE-2018-17144 and the fuzz target found the bug after ~1 day on 10 cores.
```
#2133 NEW cov: 8553 ft: 28450 corp: 659/48Kb lim: 124 exec/s: 19 rss: 110Mb L: 69/124 MS: 2 PersAutoDict-ChangeASCIIInt- DE: "\377\377\377>"-
#2151 NEW cov: 8553 ft: 28451 corp: 660/48Kb lim: 124 exec/s: 19 rss: 110Mb L: 93/124 MS: 3 CrossOver-EraseBytes-CopyPart-
#2153 NEW cov: 8553 ft: 28452 corp: 661/48Kb lim: 124 exec/s: 19 rs
...

💬 dergoegge commented on pull request "fuzz: BIP 42, BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1183501312)
`LIMITED_WHILE `?

💬 dergoegge commented on pull request "fuzz: BIP 42, BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1184774678)
Maybe we create a new setup type that doesn't have all components? (peerman, addrman, banman etc are not needed in this test)