💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634768717)
Ahh I see! Thanks for clearing it up :D I was just worried if I needed to change something from my side hehe

Thanks for all the help in this PR everyone, really learnt a great deal from each one of you!

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1634769028)
> Running this test failed on my device not sure if it's from this PR though.

It looks like you're missing the 25.0 binaries. The test is expecting to find 25.0's bitcoind at `/home/abubakarsadiq/Desktop/bitcoin/releases/v25.0/bin/bitcoind` but doesn't.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1262968377)
I don't think the specific keys are relevant.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1262971716)
I would guess that it was used in the up-downgrade test but was accidentally removed at some point. It isn't necessary anymore either as that test is now automated to go over all of the previous versions so the hardcoded specific wallet versions are no longer necessary.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1262972014)
Will do if I need to retouch.

💬 achow101 commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634808459)
If this is ready for review, please remove "[WIP]" from the title.

🤔 furszy reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1529138787)
Concept and light review ACK 31eca93a

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