💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939094701)
I think I lost it in a rebase, taken again.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939095877)
Taken

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2630466522)
Applied @vasild's suggestion: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939075014

💬 hebasto commented on issue "assumeutxo: Ensure transactions are not presented as confirmed until background sync is complete":
(https://github.com/bitcoin/bitcoin/issues/28598#issuecomment-2630494804)
From https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2611913210:

> > Reading the thread, it doesn't really seem like there is buy in for this change from other contributors or reviewers.
>
> In that case #28598 should be closed as won't fix.

💬 fanquake commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#discussion_r1939124711)
Why does this file sha256sum to something different in the Windows builds: https://github.com/bitcoin/bitcoin/actions/runs/13099284830/job/36545477809?pr=31779#step:8:925:
```bash
CMake Error at cmake/crc32c.cmake:12 (message):
D:/a/bitcoin/bitcoin/src/crc32c/CMakeLists.txt has been modified.
Call Stack (most recent call first):
CMakeLists.txt:594 (include)

-- Configuring incomplete, errors occurred!
```

💬 hebasto commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#discussion_r1939135054)
I think this happens due to the [Windows-style line endings](https://stackoverflow.com/questions/10418975/how-to-change-line-ending-settings).

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2630545071)
Github doesn't seem to trigger a notification if you only push commits, so it's better to also write a comment. Will review shortly.

📝 russeree converted_to_draft a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
### **Issue:**

Simply `DecodeDestination` does not handle errors where the user inputs a valid address for the wrong network. _e.x. testnet while running client on mainnet_

The current error `not a valid Bech32 or Base58 encoding` for a valid address on a different network is entirely incorrect. This is because of the is_bech32 variable used at the core of `DecodeDestination` logic only checks that the prefix matches. If it doesn't it just starts running everything as `DecodeBase58Check
...

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#issuecomment-2630633024)
ACK d5e28457a099cd546e757984043f28ba9f6689b1 modulo fixup squash

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1939171395)
In 6eddf8ea9fce23b3e6d1c7e73d7f39e839beb3b5 "migration: Skip descriptors which do not parse" typo: `hav`

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1939194885)
Ah I misunderstood the comment. It's not referring to "past bugs" that somehow left bad scripts in the wallet, but rather it's saying that `InferDescriptor` is potentially unreliable.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939205224)
Can there be a test that enforces `pblocktemplate->vTxFees[0] = -nFees`? Ie the test to fail if that line is removed?

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939240601)
> but in that scenario there can't be a `BlockTemplate` ...

What about

1. A `BlockTemplate` is generated
2. Shutdown is initiated
3. This code is reached and `TipBlock()` returns an empty optional, triggering the `Assume()`

Can't happen?

💬 hebasto commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#discussion_r1939241677)
Fixed in the recent push.

👍 vasild approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2589664657)
ACK 276ce2eea38865154017047f3761225c2b504cf6

Two things that would be nice to figure out before merge:

1. There could be a low-hanging fruit to avoid iterating over all transactions and use `-nFee` which should be significantly faster: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937311760

2. Just to confirm that the `Assume()` can't happen in normal circumstances: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939240601

💬 hebasto commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#issuecomment-2630727456)
@purpleKarrot
> Whether the upstream `CMakeLists.txt` file is used or not, the `crc32c` subtree should be added with `add_subdirectory` and not `include`. The latter prevents variables from leaking into the parent directory.

Certainly, I agree with keeping things as scoped as possible.

The initial plan was to replace `include(cmake/crc32c.cmake)` with `add_subdirectory(src/crc32c)`. However, the perspective has since [changed](https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-24
...

🤔 rkrux reviewed a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2589566638)
Build and tests successful at e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc
Some initial comments, will give it another pass again.

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939235575)
Can this happen for internal keychain as well?

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939203297)
I had been wondering why such a path can't occur besides this case, this comment in `DeriveNewChildKey` function answers it: https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/scriptpubkeyman.cpp#L1129

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939235210)
I spent some time trying to understand why this one is picked here only to realise later that 9 is the last index at this chain because of keypool size set to 10 at the top of this test here: https://github.com/bitcoin/bitcoin/pull/29124/commits/e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc#diff-b5055a65809b46e2775f6e28aa6eb8171e3fcc7a535cce5687a548b9ff233669R44

This 9 and 10 can be tied to each other with a constant for better readability.