🚀 achow101 merged a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463)

💬 furszy commented on pull request "fees: prevent redundant estimates flushes":
(https://github.com/bitcoin/bitcoin/pull/32748#discussion_r2183951988)
could write this in a single line instead.
```c++
if (WITH_LOCK(m_cs_fee_estimator, return !MaxUsableEstimate())) return;
```

Still, it would be nice to explain why `MaxUsableEstimate` returning 0 means "nothing to do". It is not immediately clear to me without checking the `MaxUsableEstimate` internals.

💬 davidgumberg commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#discussion_r2183963160)
```suggestion
assert {"height": 13, "hash": block.hash, "branchlen": 1, "status": "headers-only"} in self.nodes[1].getchaintips()
```

💬 davidgumberg commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#discussion_r2183963930)
nit:

```suggestion
node.bumpmocktime(200 * 60 * 2)
```

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183965661)
In 146b133697f07c8dda004d60087505b2e1ec14b9 "test: Migration of a wallet ending in `/`"

Could add a check for the expected backup path.

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183964782)
In e7fec273593d825e0e7dbec46a026aacc58c479f "test: Migration fail recovery w/ `../` in path"

I think `failed_watchonly` is the wrong wallet name. Probably best to do `f"{wallet_name}_watchonly"`

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183965792)
In 9fa5480fc4c46fa11345c45fbe4dd21c127e3e05 "test: Migration of a wallet ending in `../`"

Could add a check for the expected backup path.

💬 davidgumberg commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3034004122)
Maybe a dumb question, but why not relax the other side here? I don't know what the new criteria would be, but in an ideal world, nodes should not be sending block announcements for blocks they have identified as invalid just to avoid upsetting their peers, so maybe the constraints set by `PeerManagerImpl::SendMessages()` are too rigid?

https://github.com/bitcoin/bitcoin/blob/e3f416dbf7633b2fb19c933e5508bd231cc7e9cf/src/net_processing.cpp#L5865-L5873

💬 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