💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2630417340)
I suggest assigning the "29.0" milestone to this PR, as there is currently no CI job that tests binaries similar to our Windows release ones.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1939064794)
According to developer notes, `LogError` is for:

> severe problems that require the node (or a subsystem) to shut down
entirely

And IIUC we indeed shut down here.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939070752)
Will push fix if I need to change anything else.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939075014)
Consider the following. It avoids the hackish part and achieves the same purpose:

```diff
diff --git i/src/node/interfaces.cpp w/src/node/interfaces.cpp
index 9fe4984cd0..8022b47b5e 100644
--- i/src/node/interfaces.cpp
+++ w/src/node/interfaces.cpp
@@ -960,13 +960,13 @@ public:
// could have been generated before a tip exists.
Assume(tip_block);
tip_changed = tip_block != m_block_template->block.hashPrevBlock;
return tip_changed;

...

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1939076473)
Agreed.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939077245)
This is marked as resolved, but the code is not changed.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1939079280)
Thanks for clarifying. So in that case both too many and too few bytes are prevented.

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