💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2750558547)
Rebased (just in case), added testnet4 magic bytes coverage to `feature_assumeutxo.py` (new commit), removed the duplication in `argsman_tests.cpp`.
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2750558547)
Rebased (just in case), added testnet4 magic bytes coverage to `feature_assumeutxo.py` (new commit), removed the duplication in `argsman_tests.cpp`.
💬 maflcko commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2750566651)
Also, for libc++ you are enabling the strongest non-abi breaking one, so it would be better to do the same for gcc, which probably is `_GLIBCXX_DEBUG_PEDANTIC`?
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2750566651)
Also, for libc++ you are enabling the strongest non-abi breaking one, so it would be better to do the same for gcc, which probably is `_GLIBCXX_DEBUG_PEDANTIC`?
💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2011661090)
it doesn't matter right now, but this is too strict. `skip_test_if_missing_module` will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.
It would be better to just guard the specific test case that needs a wallet, so that newly added non-wallet test cases can be run without the wallet.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2011661090)
it doesn't matter right now, but this is too strict. `skip_test_if_missing_module` will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.
It would be better to just guard the specific test case that needs a wallet, so that newly added non-wallet test cases can be run without the wallet.
💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2011662736)
> I tried to use names similar to BIP-345
The terms `{vault,trigger,withdrawal,recovery} transaction` from that BIP are indeed useful. But I found it a bit easier to follow when calling the relevant keys "hot" and "cold" and not having a third one.
> You could, but I don't think that makes a lot of sense in practice, as the alternate_pk has no covenant restriction.
Maybe I'm still confused then. In my branch I renamed `recover_pk` to `cold_pk`. So the cold key can be used for keypath s
...
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2011662736)
> I tried to use names similar to BIP-345
The terms `{vault,trigger,withdrawal,recovery} transaction` from that BIP are indeed useful. But I found it a bit easier to follow when calling the relevant keys "hot" and "cold" and not having a third one.
> You could, but I don't think that makes a lot of sense in practice, as the alternate_pk has no covenant restriction.
Maybe I'm still confused then. In my branch I renamed `recover_pk` to `cold_pk`. So the cold key can be used for keypath s
...
💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011687872)
Exactly, from what I could see it would be set from `ArgsManager::GetChainTypeString()` in one case and `CChainParams::GetChainTypeString()` in another.
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011687872)
Exactly, from what I could see it would be set from `ArgsManager::GetChainTypeString()` in one case and `CChainParams::GetChainTypeString()` in another.
⚠️ maflcko opened an issue: "fuzz: psbt_base64_decode fails on Windows"
(https://github.com/bitcoin/bitcoin/issues/32135)
https://github.com/bitcoin/bitcoin/actions/runs/14046520411/job/39356515207
```
Assertion failed: DecodeBase64PSBT(psbt, random_string, error) == error.empty(), file D:\a\bitcoin\bitcoin\src\test\fuzz\base_encode_decode.cpp, line 95
Error processing input "D:\\a\\_temp\\qa-assets\\fuzz_corpora\\psbt_base64_decode\\0041ae3fba09a33e18a330c95acce904552b7114"
(https://github.com/bitcoin/bitcoin/issues/32135)
https://github.com/bitcoin/bitcoin/actions/runs/14046520411/job/39356515207
```
Assertion failed: DecodeBase64PSBT(psbt, random_string, error) == error.empty(), file D:\a\bitcoin\bitcoin\src\test\fuzz\base_encode_decode.cpp, line 95
Error processing input "D:\\a\\_temp\\qa-assets\\fuzz_corpora\\psbt_base64_decode\\0041ae3fba09a33e18a330c95acce904552b7114"
💬 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.
(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.
(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.
(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
(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
...
(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)
(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.
...
(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.
(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).
(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)
(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.
(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)
(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
...
(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/*)
```
(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/*)
```