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

👍 willcl-ark approved a pull request: "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/25193#pullrequestreview-1413010190)
ACK 974140f9e7

Can confirm via testing that this fixes the majority of #27558, although I was not able to reliably abort my node _during_ an `invalidateblock` call as OP did in that issue...

I also extracted the feature_coinstatsindex test from 5fafeec471 and checked that it did indeed fail without the corresponding changes to `BaseIndex::Init()`.

🤔 hebasto reviewed a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1413008558)
Approach ACK db668b3644883c07064b46b4e2cfd269ac9dffbd.

Changes in `test/util/common.cpp` does not seem required in the dbd739d54386dcd5dd1d945423bc82362263d4e8 "refactor, BlockManager: Replace fastprune from arg with options" commit. Maybe combine them into the following commit?

💬 hebasto commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184962143)
a614ac59cd753c7ef83899602e0176654c5fff87, nit, naming:
```suggestion
virtual bool getBlockByIndex(CBlock& block, const CBlockIndex* index) = 0;
```

💬 hebasto commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184963724)
6b9ba7b54097bb32d3c1596b7fdcec4428c27a3a

Isn't capturing `[&get_block_by_index]` enough?

🤔 dergoegge reviewed a pull request: "refactor: Remove need to pass chainparams to BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/27570#pullrequestreview-1413028306)
Concept ACK

Should we do the same for `Consensus::Params`?