💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2127492669)
This is just addressing a specific comment from #29668: https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1651138486 I'm not sure which CI you mean? But anyway I think I don't want to extend the scope by adding more include fixes, I can drop these if you want.

💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2127497118)
Right, good catch on the pointers!

I can try to add such a unit test, do you have specific scenarios in mind that you would like to see covered? If we just want to make sure this block is covered I think I could also add a functional test that basically executes the edge case that @furszy described above: https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2029126932 and I think that should be fairly easy to do.

💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2127498222)
Makes sense, I am keeping the lock for the later calls as well now.

💬 achow101 commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2941640165)
ACK f98e1aaf34e347088caa54403521e3b5cb55dd40

💬 TheCharlatan commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2127501557)
The tidy job includes a complete run of include what you use suggestions for all source files.

🤔 jonatack reviewed a pull request: "doc: update tor docs to use bitcoind binary from path"
(https://github.com/bitcoin/bitcoin/pull/32679#pullrequestreview-2898210113)
ACK 4ce53495e5e18370b7935551b3b8700faa720a33

🚀 achow101 merged a pull request: "rpc: Note in fundrawtransaction doc, fee rate is for package"
(https://github.com/bitcoin/bitcoin/pull/32607)

💬 achow101 commented on pull request "test: apply microsecond precision to test framework logging":
(https://github.com/bitcoin/bitcoin/pull/32676#issuecomment-2941703254)
ACK ed179e0a6528c39b3bca76365f256716f917e19b

🚀 achow101 merged a pull request: "test: apply microsecond precision to test framework logging"
(https://github.com/bitcoin/bitcoin/pull/32676)

💬 b-l-u-e commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2941773645)
I've been working on this same issue and my findings align well with @vasild feedback for a simpler approach.

**Regarding the core fix**: @vasild suggested approach is exactly right - the fix can be much simpler without the code duplication issues. Here's what I implemented successfully:
bitcoin/src/init.cpp
```cpp
// We should discover addresses in two cases:
// 1. When no -bind or -whitebind is specified (bind_on_any is true)
// 2. When any -bind address is 0.0.0.0 or ::
bool shou
...

💬 achow101 commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2941859685)
ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351

🤔 w0xlt reviewed a pull request: "test: wallet: cover wallet passphrase with a null char"
(https://github.com/bitcoin/bitcoin/pull/32675#pullrequestreview-2898357025)
Code review ACK https://github.com/bitcoin/bitcoin/pull/32675/commits/7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

👋 achow101's pull request is ready for review: "wallet: Fix wallet interface detection of encrypted wallets"
(https://github.com/bitcoin/bitcoin/pull/32620)

💬 achow101 commented on pull request "wallet: Fix wallet interface detection of encrypted wallets":
(https://github.com/bitcoin/bitcoin/pull/32620#issuecomment-2942015745)
Ready for review now.

💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2124996445)
non-blocking suggestion related to my earlier comment about `Assume` in the constructor: What if instead of a reference to a `CTxOut` WalletTXO just kept the vout index of the output. Not even really for performance reasons, but just because it more tightly represents / enforces the meaning/usage of `WalletTXO` This is an implementation detail of `WalletTXO`, and would just be handled by `GetTxOut()`, so if it makes sense, it can be done in a follow-up.

💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2127237787)
I'm a little bit unsure of whether or not this optimization is correct.

My understanding is that in the loop below, a transaction will only be inserted into `trusted_parents` if it, and all of its parents are trusted:

https://github.com/bitcoin/bitcoin/blob/72378b2bf42714b67fb3f0272b5fbb7c37a08898/src/wallet/receive.cpp#L283-L285

But the "cached" checks, so to speak, are all of the checks above the `trusted_parents.count()` check in the loop:

https://github.com/bitcoin/bitcoin/blob/7
...

💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2125040229)
Is it possible for an outpoint to ever change from something else to `ISMINE_NO`? I'm wondering if there should be a `m_txos.contains(outpoint)` check and `SetIsMine()` update like happens below for the `ISMINE_NO` case.

💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2127103860)
In https://github.com/bitcoin/bitcoin/pull/27286/commits/72378b2bf42714b67fb3f0272b5fbb7c37a08898 (_wallet: Exit IsTrustedTx early if wtx is already in trusted_parents_):

nanonit: commit message `IsTrustedTx` -> `TxIsTrusted`

💬 cmdruid commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2942855381)
I apologize for the wait.

So far I have been using this command to great effect on my personal fork. It does exactly what I need it to do, and seems to be the best way to go about implementing this kind of command.

I did not make any changes to `abandontransaction` though. It doesn't apply to my use-case, and it also doesn't cover transactions in the mempool that are unrelated to my wallet (I want this command to purge *all* transactions).

I would like to submit it as-is, but I need to
...

💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128151655)
Verified building from source works for the most recent release listed in get_previous_releases.py:
```sh
$ git checkout cbd8e3d51148d29dca576982de4c4398bf3e2f6f # parent commit of this PR
<...> # modify `make` -> `make -j...` in get_previous_releases.py
$ ./test/get_previous_releases.py v28.0 # builds successfully from source
$ ls releases/v28.0/bin/
bitcoin-cli bitcoin-tx bitcoind
```