💬 MarcoFalke commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262932762)
Thanks, done. Also used C++ 17 structured binding declaration to get the map key-value.

💬 MarcoFalke commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262934214)
```suggestion
const auto it_ins{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))};
```

Haven't tracked down which C++20 LWG paper this was. Let me know if someone finds something.

💬 fanquake commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634723100)
> [Use of -O2 and above is not recommended as Memcheck occasionally reports uninitialised-value errors which don't really exist.](https://valgrind.org/docs/manual/quick-start.html)

I guess this doesn't matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.

I think there's another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.

💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634727453)
> I guess this doesn't matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.

Only for gcc, see https://github.com/bitcoin/bitcoin/issues/27741

> I think there's another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.

Right. Probably not worth to spend time here, with the switch to cmake. But that makes me wonder how cmake handles this.

👍 hebasto approved a pull request: "guix: Remove librt usage from release binaries"
(https://github.com/bitcoin/bitcoin/pull/28069#pullrequestreview-1529066233)
ACK 8f6f0d81ee3a9ea582e9c9cf986613da86760098

🤔 furszy reviewed a pull request: "[WIP] descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1529085037)
> @furszy I think there is only a downgrade issue, no? If someone were to import a sh(raw(...)) as watch-only address in a descriptor wallet after allowing that, and then downgrades to older software which does not support it.

Yeah good @sipa and @achow101. We are sync then.

My point was more about agreeing that the bug fix should go first, so it can be backported for users of v24 and v25. Preventing them from entering an unrecoverable state after migration.

And then we can move forward
...

💬 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-1634760577)
Hello, just wondering if there is something left from my side, or is this ready for review and merging?

💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634766029)
>

Bitcoin development is always bottlenecked and all changes no matter how small must thoroughly reviewed for and allowed time for social consensus since every change could potentially cost people real world value. You can work on other issues and/or await more feedback.

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