💬 maflcko commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r2011712656)
Looks like this newly added assertion fails for some reason on Windows. I don't have Windows, so I can't debug this. See https://github.com/bitcoin/bitcoin/issues/32135.

💬 maflcko commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2011716063)
The getters return a value, not a pointer or reference. C++ allows to bind the return value to either a reference or a value, but the end result is the same and either is fine here in this context.

However, the type is not `std::string` as claimed by you. (This can be seen by trying to compile your suggestion with a C++ compiler and observing the compile error).

Also, the type wasn't mentioned previously in the code for most of the GetArg calls, so it seems better to leave it this way.

💬 maflcko commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2011716174)
Yes, I like scope braces, so that the scope is limited for symbols inside the scope. Also, to retain the prior scope like is was previously in the code.

💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011760310)
(Inline comment at random position):

Noticed doc/REST-interface.md still mentions testnet3 port:
https://github.com/bitcoin/bitcoin/blob/4e6e73a3b2c5c3a4a07683bc3a6b59f8e27b78f3/doc/REST-interface.md?plain=1#L6-L7

💬 i-am-yuvi commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2750752881)
Router Model: JCO4032
System: MacOS Sequoia 15.2
Version tested: v29.0rc2
Result: Failed - No PCP response

```
2025-03-25T10:04:47Z [net] pcp: Timeout
2025-03-25T10:04:47Z [net] pcp: Retrying (1)
2025-03-25T10:04:48Z [net] pcp: Timeout
2025-03-25T10:04:48Z [net] pcp: Retrying (2)
2025-03-25T10:04:49Z [net] pcp: Timeout
2025-03-25T10:04:49Z [net] pcp: Giving up after 3 tries
2025-03-25T10:04:49Z [net] portmap: gateway [IPv6]: fe80:...
2025-03-25T10:04:49Z [net] pcp: Requesting port mapping for
...

💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011767375)
Speaking of things I'd like to delete entirely :-) (but won't)

💬 laanwj commented on issue "v29.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2750762673)
✅ i tested that the new NATPMP implementation works with protonvpn's automatic port forwarding:
```
# ip netns exec bitcoin-test bin/bitcoind -regtest -natpmp=1 -debug=net
⋮
2025-03-25T10:09:20Z mapport thread start
2025-03-25T10:09:20Z [net] portmap: gateway [IPv4]: 10.2.0.1
2025-03-25T10:09:20Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 18444 from gateway 10.2.0.1
2025-03-25T10:09:20Z Bound to 127.0.0.1:18445
2025-03-25T10:09:20Z [net] pcp: Internal address after connect: 10.2.
...

💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2750766691)
I plan to retest this with v28 to try and determine if this is a router issue, or incompatibility with the (new) implementation in v29.

💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2011796654)
>> I tried to be smart and also use -1 instead of cold_pk in recover_leaf, but that doesn't work. Not sure why.

>Putting -1 means in the pk argument means that you reuse the taproot internal key for your output

I'm still not sure if I understand this. I assumed that the taproot internal key is unchanged by the trigger transaction, i.e. it's still the cold key (in my branch).

💬 maflcko commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2750818047)
(ci failure is unrelated and can be ignored for now, see https://github.com/bitcoin/bitcoin/issues/32135)

💬 maflcko commented on issue "fuzz: psbt_base64_decode fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2750871948)
I pushed https://github.com/bitcoin-core/qa-assets/commit/c62dc520e0c7caee3e50c9ba8a88056c7f4d357f to temporarily unbreak the CI.

It would be good if someone on Windows helped here to track this down.

💬 hodlinator commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011843792)
(Done https://github.com/bitcoin-dot-org/developer.bitcoin.org/pull/291)

🤔 l0rinc requested changes to a pull request: "descriptors: Multipath/PR 22838 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2713105661)
Could we separate the concerns of `ParseKeyPath` (validation from parsing)?
And I think there may be a typo in the doc.

<details>
<summary>Suggestions</summary>

```patch
diff --git a/doc/descriptors.md b/doc/descriptors.md
index d5e845273a..60d3e0159f 100644
--- a/doc/descriptors.md
+++ b/doc/descriptors.md
@@ -288,11 +288,11 @@ For example, a descriptor of the form:

multi(2,xpub.../<0;1;2>/0/*,xpub.../<2;3;4>/*)

-will expand to the 3 descriptors
+will expand to the fo
...

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011731171)
shouldn't the wildcard have its own position in the hierarchy?
```suggestion
multi(2,xpub.../2/0/*,xpub.../4/*)
```

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011736752)
nit: I had to check the type of `value` to make sure this isn't needlessly copied - to make it slightly simpler to review, consider:
```suggestion
for (const auto& value : multipath->values) {
```

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011851451)
nit: since we're already touching this line we might as well format it

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011759617)
I don't like this third commit (nor the original implementation), we're looping over the splits, assigning a multipath if found, checking if it was already found during previous iterations, setting errors to output parameters and returning early, if we didn't return we assume it was correct, etc - it's too stateful, we're mixing not-strictly-related operations in a single loop, i.e. validation with usage, we should be able to separate the two concerns (it's a tiny loop).

👍 hebasto approved a pull request: "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain"
(https://github.com/bitcoin/bitcoin/pull/31849#pullrequestreview-2713381206)
ACK 963355037fe78eb4fbdda8631ac05a7b07fcec8c, tested on Ubuntu 24.10.

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011902071)
Yes, I believe it should.

🤔 glozow reviewed a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2713564288)
ACK 226d81f8b708ea1c3a39ec58446e291fe7440fdd

Thanks for adding the test! Checked that it'd catch any errors in the RPC code.