💬 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

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185072414)
use named tuple args where you can

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185073629)
Can you leave comments on each step to make it clear what's happening? It's been a few minutes of staring and I'm not quite sure what's exactly being tested here, making it difficult to update in the future.

My basic test flow would probably follow the lines of:

1) pick a node that doesn't have a wallet
2) make a single wpkh utxo
3) make single input psbt with that utxo
4) make sure it signs it when expected, and finalizes when expected (e.g., give it a descriptor unrelated to the utxo,
...

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

🤔 theuni requested changes to a pull request: "Enable -Wstring-concatenation and -Wstring-conversion on clang builds"
(https://github.com/bitcoin/bitcoin/pull/26288#pullrequestreview-1413259393)
Concept ACK. This has been opened for a while, I'm curious to know if any new violations have snuck in since?

💬 theuni commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#discussion_r1185107526)
Likewise here.

💬 theuni commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#discussion_r1185107056)
Could you please split this up like this instead?
```c++
auto foo = ReadRawBlockFromDisk(...);
assert(foo);
```
That way the we don't introduce a new side-effect of `NDEBUG=1`

💬 theuni commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#discussion_r1185113523)
Imo it'd be cleaner and more readable to just leave the logic here as before, turning the wonky string assertions into comments.