💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183974514)
I add this check to `migrate_and_get_rpc()` in 55e60bd9c76:

https://github.com/bitcoin/bitcoin/blob/9fa5480fc4c46fa11345c45fbe4dd21c127e3e05/test/functional/wallet_migration.py#L138-L146

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183974714)
See above: https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183974514

💬 achow101 commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-3034010351)
> @achow101 re: slow preferred peer "causing congestion" -- wouldn't this only affect the single user who explicitly set the preferred peer?

Yes, however I have seen people post on various forums suggestions of specific nodes that they can addnode, and users may be addnode'ing such nodes without realizing that there are potential adverse consequences of doing so. Generally, I would prefer to have things that change how we treat nodes be limited to the whitelist options rather than addnode als
...

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183977261)
Thanks for catching this, I've applied your fix in f67d6fe811

💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3034576513)
Addressed @instagibbs [suggestion](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2180262495) to use time-based timelock instead of n block method.

💬 Sjors commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3034701850)
From CI:

```
assert not "non_witness_utxo" in mining_node.decodepsbt(signed_psbt["psbt"])["inputs"][0]
```

I didn't check the code, but that's usually a cryptic way of saying that signing the psbt failed.

💬 b-l-u-e commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3034729829)
```
@vasild Thanks for the suggested improvements! I have a question about the `self.extra_args` configuration.

In your patch, node 2 only has `-whitebind=0.0.0.0` without any onion bind:
```python
['-discover', f'-whitebind=0.0.0.0:{self.bind_ports[2]}'], # Explicit -whitebind=0.0.0.0
```

However, when I tried this configuration, I encountered port conflicts because the test framework automatically adds an onion bind to node 2, which conflicts with the port already used by node 1.

...

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184561400)
In 03f585d58ccf4d5c02d621c5b6046d45807b3201 _bitcoin wrapper: improve help output_: users might be confused by the distinction between no command, `--help` and `help`.

That's because if a user starts with "bitcoin" then "this help message" implies there's nothing additional to see. Hopefully they'll still be encouraged to try by the sentence at the end ("Run 'bitcoin help' to see additional commands").

But maybe a better phrasing here is: "List all available commands."

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184594801)
In 03f585d58ccf4d5c02d621c5b6046d45807b3201 _bitcoin wrapper: improve help output_: this is a nice improvement.

Before:

```
bitcoin --help
Usage: bitcoin [OPTIONS] COMMAND..

which bitcoin
/usr/local/bin/bitcoin

/usr/local/bin/bitcoin --help
Usage: /usr/local/bin/bitcoin [OPTIONS] COMMAND...
```

After:

```
/usr/local/bin/bitcoin --help
Usage: bitcoin [OPTIONS] COMMAND..
```

Only tested on macOS

👍 Sjors approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2985811000)
re-ACK 03f585d58ccf4d5c02d621c5b6046d45807b3201

Left a suggestion for slightly improved wording.

💬 Sjors commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32871#issuecomment-3034796044)
TIL it's possible to make a PR with 0 commits in it. Did you mean to open an issue?

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3034814885)
Rebased after https://github.com/bitcoin/bitcoin/pull/29307

✅ maflcko closed a pull request: "New SVG, Icons, PNGs and X PixMaps"
(https://github.com/bitcoin/bitcoin/pull/32871)

💬 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32871#issuecomment-3034899121)
@Sjors @maflcko the pull (bot) destroyed my PR. I see some of my changes reverted here: https://github.com/bitcoin/bitcoin/compare/a7270b8fb8fd6c92d88de87de492cf88aef1eda7..e3f416dbf7633b2fb19c933e5508bd231cc7e9cf

I use a the pull github app to keep the master branch up to date with bitcoin/bitcoin/master maybe that caused the issue. I will turn it off and send a new PR.

🤔 stratospher reviewed a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-2946966884)
ACK 11a9d55e.

Agree that applying reindex-assumevalid combo can be useful in the special cases mentioned above to speedup reindexing when we don't have all blocks.

A behaviour change during assumevalid+reindex now would be:
- we'd skip script checks even when the assumevalid blockindex doesn't exist (ex:we put `assumevalid=somegarbage(or)blockhash-with-typo`)
- this was not the case on master where we skip script checks only when know that the assumevalid blockindex exists.

Don't thin
...

💬 stratospher commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2184580600)
any particular reason for having this high minimum chain work? (ex: for 32256 blocks instead of 3256 blocks). also curious how you computed it?

💬 stratospher commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2184701733)
the tests now take 2m34.878s on my system (was around 18 seconds before).
since the tests takes much more time now, maybe remove it from the "Tests less than 30s" section in `test_runner.py`?

💬 fanquake commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-3034989842)
> Same here, fwiw, running with --random=3450808900320758527 didn't fail for me on master (at commit https://github.com/bitcoin/bitcoin/commit/bf75c9964fb28a5e9b8a07097248ffa960738478).

This is the one that fails for me on 29.x. Backported to 29.x in #32863.

💬 m3dwards commented on pull request "ci: update pwsh to use custom shell that fails-fast":
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3035043670)
> 1. The output in the "Get tool information" step has changed:

I just added this as a nice to have but happy to remove.

> 2\. The exit code is not propagated to the action's status.

Unfortunately I think this is just how it works. When there is a failure it will always use code 1. Without having to write code that checks error codes and returns those codes each time we call a native executable I don't think it's possible to have the same error code.

💬 maflcko commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2184792071)
Yeah, seems fine to keep this at the end for users and devs that have it not set? Maybe just use a trivial one-line patch?

```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4da12e0f46..c3dea019b4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -710,6 +710,7 @@ if(configure_warnings)
message(WARNING "${warning}")
endforeach()
message(" ******\n")
+ message(AUTHOR_WARNING "Warnings have been encountered!")
endif()

# We want all build properties to be encapsul
...