📝 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`?

💬 fanquake commented on pull request "ci: set docker run --ulimit to workaround Valgrind assertion":
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1534732109)
Migrated to podman on 38.

✅ fanquake closed a pull request: "ci: set docker run --ulimit to workaround Valgrind assertion"
(https://github.com/bitcoin/bitcoin/pull/27364)

💬 willcl-ark commented on issue "Should Bitcoin Core quit when something goes wrong?":
(https://github.com/bitcoin/bitcoin/issues/22398#issuecomment-1534734355)
@GaylordTuring, are you able to reproduce this issue with a current version of Bitcoin Core e.g [24.0.1](https://bitcoincore.org/en/download/)?

If you are it would be helpful to reproduce it when running `bitcoind` with the `-debug=1` option, so we can get closer to diagnosing what is causing the crash without a relevant error message, otherwise it will be difficult for anyone to help you.

I also notice in your logs the lines:

```
23:2021-06-13T12:23:48Z BerkeleyEnvironment::Open: LogD
...

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1184989106)
This is called `pindex` in the original: https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.h#L235 . Is that wrong?

💬 hebasto commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1185002800)
No, it is not wrong. Just mentioned it as in new code we do not make a reference to a type of the variable in its name:
```
git grep -e "CBlockIndex\* index"
```

💬 instagibbs commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1534767675)
> We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not the ones we broadcasted to (via short-lived connection).

Ideally, if outbound has clearnet connections, we would resume normal operation IFF one of those connections INV's it. This reduces risk of sybil influencing the node.

👍 furszy approved a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1413069965)
ACK 6f1d3948

💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1185000020)
Let me see if I'm following this:

`Tx_AB1` spends B, and `Tx_ABC2` (the child of `Tx_AB1`) spends C. So, when the double spend tx gets disconnected, `Tx_AB1` state changes to `inactive`, marking output B and C as spent again.

Which ends up with the txes stuck now right?, and the user need to manually re-submit them.

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

Updated db668b3644883c07064b46b4e2cfd269ac9dffbd -> 9033dc6827cc623f1f7176fde120229f36ff5e74 ([removeBlockstorageArgs_18](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_18) -> [removeBlockstorageArgs_19](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_19), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_18..removeBlockstorageArgs_19))

* Addressed @hebasto's [comment](https://github
...

💬 MarcoFalke commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1534824518)
> Should we do the same for Consensus::Params?

Yes, this is what this pull is doing.