💬 furszy commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1262989504)
> Naively, I'd think ProcessNewBlock should just do if (m_interrupt) return false; and the types of races you mention would be avoided.

Hmm, if it arrived to `ProcessNewBlock` is because the node requested the block. So I would try to not discard it, and instead make it pass through `AcceptBlock` so it can be stored. Then quit early before ABC.

🤔 ishaanam reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-1527016849)
Concept ACK

Still working on reviewing this, but here are some initial things. Thanks for breaking it up into twelve commits, it makes it easy to review.

💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1261601507)
In 68bb1463dc1f995fbfdb75d3acef625bde104275 "wallet: Change balance calculation to use m_txos "

nit:
```suggestion
const bool is_trusted{CachedTxIsTrusted(wallet, txo.GetWalletTx())};
```
& also remove the unused `trusted_parents` from above. (for `GetAddressBalances` as well)

💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1262864895)
In 3f3246efe299552e450362a03c61c602a168f7de "wallet: Recalculate the wallet's txos after any imports "

Does this also need to be called during `sethdseed` and `keypoolrefill`.

💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1262959018)
In https://github.com/bitcoin/bitcoin/commit/881650ed319c6ed74e66ae56c34cdee93125231b " wallet: Use wallet's TXO set in AvailableCoins "

Can't `tx_ok` still be false at this point if a transaction that has a depth of 0 and is not in the mempool has two txos that belong to the wallet? Since then when the first txo is evaluated, a `{false, false}` entry will be created for that transaction hash, and then when the second txo is evaluated, that entry will be used because the mempool check will be
...

💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1262884442)
In 881650ed319c6ed74e66ae56c34cdee93125231b " wallet: Use wallet's TXO set in AvailableCoins "

Shouldn't txos in `m_txos` never be `ISMINE_NO`?

```suggestion
assert(mine != ISMINE_NO);
```

💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1262869959)
In https://github.com/bitcoin/bitcoin/commit/3f3246efe299552e450362a03c61c602a168f7de "wallet: Recalculate the wallet's txos after any imports "

Is this needed if the wallet will rescan anyways?

💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1262844660)
In 68bb1463dc1f995fbfdb75d3acef625bde104275 " wallet: Change balance calculation to use m_txos "

nit: checking the tx depth is redundant here because if the transaction is `TxStateMempool` then `GetTxDepthInMainChain` will always return 0.
```suggestion
} if (!is_trusted && txo.GetWalletTx().InMempool()) {
```

💬 achow101 commented on pull request "fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`":
(https://github.com/bitcoin/bitcoin/pull/27549#issuecomment-1635038539)
ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838

🚀 achow101 merged a pull request: "fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`"
(https://github.com/bitcoin/bitcoin/pull/27549)

💬 tobtoht commented on pull request "guix: Remove librt usage from release binaries":
(https://github.com/bitcoin/bitcoin/pull/28069#issuecomment-1635059758)
Guix builds:

```
$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
67078bddd5dc32801b8c916c3bc12f1404da572312f0158a89b9603c1f753969 guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/SHA256SUMS.part
794dd00009860fd67d7e51463ee1c5ea9677dfff1c739dd0b91cf73136deb655 guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/bitcoin-8f6f0d81ee3a-aarch64-linux-gnu-debug.tar.gz
eb9cf3f472ffbc37446fe4d80fe81dc62cf1c28c4d57dd8a7b7176e654
...

💬 Crypt-iQ commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1263162187)
I think because the only call to `g_requests_cv.wait(...)` has a predicate that waits for `g_requests.empty()`. I've removed the dependency in the latest patch

💬 Crypt-iQ commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#issuecomment-1635090604)
I've removed the `assert(n == 1)` in `evhttp_request_set_on_complete_cb` because of this comment: https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1611979215. It appears that both `evhttp_connection_set_closecb` and `evhttp_request_set_on_complete_cb` can run which causes a crash. I wasn't able to reproduce it and still want to know what exactly happened, but I think removing the assert here is certainly better than having a crash.

💬 Crypt-iQ commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1263163161)
done

💬 Crypt-iQ commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1263163190)
done

✅ JayBitron closed a pull request: "exclude ipc scheme from port check"
(https://github.com/bitcoin/bitcoin/pull/28020)

💬 MarcoFalke commented on pull request "refactor: Continue moving application data from CNode to Peer":
(https://github.com/bitcoin/bitcoin/pull/26621#issuecomment-1635315780)
Needs rebase if still relevant

🤔 MarcoFalke reviewed a pull request: "test: add end-to-end tests for CConnman and PeerManager"
(https://github.com/bitcoin/bitcoin/pull/26812#pullrequestreview-1529671710)
Not sure about changing the framework.

Also, needs rebase if still relevant.

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1263332063)
Not sure about changing this. It just makes it harder to use. If you prefer to not have an exception, this can use an `Assert`? Also, in the commit description, it would be good to refer to documentation why it is "not allowed", and/or include an example of what happens.

💬 MarcoFalke commented on pull request "net: Use serialization parameters for CAddress serialization":
(https://github.com/bitcoin/bitcoin/pull/25284#issuecomment-1635587006)
rebased