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

💬 MarcoFalke commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1534837584)
lgtm ACK 7e3d4f8e86e86f32d8911abd458b9e7c939ef3d5

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185038780)
use `SIGHASH_ALL` for clarity

🤔 instagibbs reviewed a pull request: "rpc: add `descriptorprocesspsbt` rpc"
(https://github.com/bitcoin/bitcoin/pull/25796#pullrequestreview-1413132311)
concept ACK, would appreciate test tighten-up for maintainability

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185068008)
key_info.privkey?

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185042239)
future work: someone make this a subroutine since this shows up in many places