💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1831172493)
This seems fine to remove because it's redundant anyways.
But worth noting in the commit message.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1831172493)
This seems fine to remove because it's redundant anyways.
But worth noting in the commit message.
💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1831252615)
tracing.md doc is still saying
> 5. Replacement transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1831252615)
tracing.md doc is still saying
> 5. Replacement transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
🤔 darosior reviewed a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2418934243)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2418934243)
Concept ACK.
💬 darosior commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1831410534)
The shallow copy in the version before this PR is causing a problem because we mess with a `Node`'s `subs` in the destructor to avoid a recursion stack overflow:
https://github.com/bitcoin/bitcoin/blob/2c90f8e08c4cf44d4c1ef3dde0e7f7991b8b9390/src/script/miniscript.h#L513-L524
In the scriptpubkeyman fuzz target using the seed from #30864 we would `Parse` the `tr(%17/<2;3>,l:pk(%08))` multipath descriptor into a `parsed_descs` vector of 2 `Descriptor` pointers. We would use the first of those
...
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1831410534)
The shallow copy in the version before this PR is causing a problem because we mess with a `Node`'s `subs` in the destructor to avoid a recursion stack overflow:
https://github.com/bitcoin/bitcoin/blob/2c90f8e08c4cf44d4c1ef3dde0e7f7991b8b9390/src/script/miniscript.h#L513-L524
In the scriptpubkeyman fuzz target using the seed from #30864 we would `Parse` the `tr(%17/<2;3>,l:pk(%08))` multipath descriptor into a `parsed_descs` vector of 2 `Descriptor` pointers. We would use the first of those
...
⚠️ maflcko opened an issue: "fuzz: connman target: terminate called after throwing an instance of 'std::bad_alloc'"
(https://github.com/bitcoin/bitcoin/issues/31234)
Base64 reproducer:
```
XP//////////BiAgICBbICAHAADg/4Hf394gICAgICAgIAAgICAgIHb/FiAgICAgdGggtyAgICCB
CAQAIDAXIAEAAAAAAAAAAQAAAAAAAAABAAAAAAAAgYGBgYGB7QH//2ZoZWNrcHRhYWFhYWFhl2Fh
YWFhYWFhYWFhYWFhYWFhYWFhYWFhYQAAAAAAAFxjYWFhYWFhYWFcdIFhYWFhZHJh9xP3ExPAE8BA
7/cTwBP3ExP398D3AQAAAAAAAASBgYGBgYHtAf///WdldGNmaGVja3B0YWFhYWFhYWFhl2FhYWFh
YWFhYWFhYWFhYWFhYWFhYWFhYQ==
```
```
$ sha1sum ./crash && FUZZ=connman ./bld/src/test/fuzz/fuzz ./crash
c0f5ddd240439f74d6eac83bbb67115b1ad1d209 ./crash
...
(https://github.com/bitcoin/bitcoin/issues/31234)
Base64 reproducer:
```
XP//////////BiAgICBbICAHAADg/4Hf394gICAgICAgIAAgICAgIHb/FiAgICAgdGggtyAgICCB
CAQAIDAXIAEAAAAAAAAAAQAAAAAAAAABAAAAAAAAgYGBgYGB7QH//2ZoZWNrcHRhYWFhYWFhl2Fh
YWFhYWFhYWFhYWFhYWFhYWFhYWFhYQAAAAAAAFxjYWFhYWFhYWFcdIFhYWFhZHJh9xP3ExPAE8BA
7/cTwBP3ExP398D3AQAAAAAAAASBgYGBgYHtAf///WdldGNmaGVja3B0YWFhYWFhYWFhl2FhYWFh
YWFhYWFhYWFhYWFhYWFhYWFhYQ==
```
```
$ sha1sum ./crash && FUZZ=connman ./bld/src/test/fuzz/fuzz ./crash
c0f5ddd240439f74d6eac83bbb67115b1ad1d209 ./crash
...
💬 maflcko commented on pull request "fuzz: fuzz connman with non-empty addrman + ASMap":
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2460392280)
https://github.com/bitcoin/bitcoin/issues/31234
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2460392280)
https://github.com/bitcoin/bitcoin/issues/31234
💬 mzumsande commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1831468608)
yes, happy to ack that follow-up. I think everything is fine, but my suggestion would be something like "Can't serve the connection to..." because we already log "Serving connection to..." in the success case. And maybe add another log entry "Keeping connection alive" `else` clause in the `finally` block a few lines below to avoid conditional log messages.
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1831468608)
yes, happy to ack that follow-up. I think everything is fine, but my suggestion would be something like "Can't serve the connection to..." because we already log "Serving connection to..." in the success case. And maybe add another log entry "Keeping connection alive" `else` clause in the `finally` block a few lines below to avoid conditional log messages.
🤔 vasild reviewed a pull request: "net, init: derive default onion port if a user specified a -port"
(https://github.com/bitcoin/bitcoin/pull/31223#pullrequestreview-2418908916)
Another alternative: don't listen for Tor on regtest by default (were all reports for this for regtest?).
What about setups that use e.g. `-port=5000` and have a manually configured the Tor hidden service in `torrc` that redirects to `127.0.0.1:8334`? E.g.:
```
HiddenServiceDir /var/db/tor/myservice/
HiddenServiceVersion 3
HiddenServicePort 8333 127.0.0.1:8334
```
This change would _silently_ break those setups - because we will now be listening on `127.0.0.1:5001` and other peers t
...
(https://github.com/bitcoin/bitcoin/pull/31223#pullrequestreview-2418908916)
Another alternative: don't listen for Tor on regtest by default (were all reports for this for regtest?).
What about setups that use e.g. `-port=5000` and have a manually configured the Tor hidden service in `torrc` that redirects to `127.0.0.1:8334`? E.g.:
```
HiddenServiceDir /var/db/tor/myservice/
HiddenServiceVersion 3
HiddenServicePort 8333 127.0.0.1:8334
```
This change would _silently_ break those setups - because we will now be listening on `127.0.0.1:5001` and other peers t
...
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1831444830)
I think it is simpler and is ok to use reduce all this to:
```diff
diff --git i/src/init.cpp w/src/init.cpp
index c10f792c7d..7e141dbd4c 100644
--- i/src/init.cpp
+++ w/src/init.cpp
@@ -518,13 +518,13 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
" If <type> is not supplied or if <type> = 1, indexes for all known types are enabled.",
ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-addnode=<ip>", strpr
...
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1831444830)
I think it is simpler and is ok to use reduce all this to:
```diff
diff --git i/src/init.cpp w/src/init.cpp
index c10f792c7d..7e141dbd4c 100644
--- i/src/init.cpp
+++ w/src/init.cpp
@@ -518,13 +518,13 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
" If <type> is not supplied or if <type> = 1, indexes for all known types are enabled.",
ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-addnode=<ip>", strpr
...
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1831395166)
This implies that it's not possible in v28.0, but it is if `-bind=...=onion` is used. Maybe clarify:
```suggestion
This re-allows setups with multiple local nodes using different `-port`, which
would lead to a startup failure in v28.0 if `-bind=...=onion` is not used due to a port collision error. (#31223)
```
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1831395166)
This implies that it's not possible in v28.0, but it is if `-bind=...=onion` is used. Maybe clarify:
```suggestion
This re-allows setups with multiple local nodes using different `-port`, which
would lead to a startup failure in v28.0 if `-bind=...=onion` is not used due to a port collision error. (#31223)
```
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1831404260)
If `-port=5999` is given (5999 is not bad, but 6000 is), then this would produce a warning like "-port request to listen on port 6000 ..." which could be confusing because the user did not specify 6000. But more importantly I think we don't need this bad port check at all because this is completely local thing, only between `bitcoin` and the Tor router. That port is not announced to the outside world, so it is ok to just drop check.
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1831404260)
If `-port=5999` is given (5999 is not bad, but 6000 is), then this would produce a warning like "-port request to listen on port 6000 ..." which could be confusing because the user did not specify 6000. But more importantly I think we don't need this bad port check at all because this is completely local thing, only between `bitcoin` and the Tor router. That port is not announced to the outside world, so it is ok to just drop check.
💬 brunoerg commented on issue "fuzz: connman target: terminate called after throwing an instance of 'std::bad_alloc'":
(https://github.com/bitcoin/bitcoin/issues/31234#issuecomment-2460422584)
Working on it. We should limit `max_addresses` and `max_pct`.
(https://github.com/bitcoin/bitcoin/issues/31234#issuecomment-2460422584)
Working on it. We should limit `max_addresses` and `max_pct`.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2460441138)
> The current implementation seems to be using the aggregate pubkey (before the tweaks) inside the key of the `PSBT_IN_MUSIG2_PUB_NONCE`
Indeed, fixed.
> Therefore, I interpreted it as the taproot pubkey for a keypath spend
I've interpreted (and implemented) it as also allowing the taproot internal key, not just the output key.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2460441138)
> The current implementation seems to be using the aggregate pubkey (before the tweaks) inside the key of the `PSBT_IN_MUSIG2_PUB_NONCE`
Indeed, fixed.
> Therefore, I interpreted it as the taproot pubkey for a keypath spend
I've interpreted (and implemented) it as also allowing the taproot internal key, not just the output key.
🤔 1440000bytes requested changes to a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-2419071104)
NACK
This pull request and related efforts are bad for bitcoin in lot of ways not just partition risk.
We need to fix other things before trying to achieve this. Disappointed that some devs keep repeating it will hide that you are running a node.
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-2419071104)
NACK
This pull request and related efforts are bad for bitcoin in lot of ways not just partition risk.
We need to fix other things before trying to achieve this. Disappointed that some devs keep repeating it will hide that you are running a node.
👋 achow101's pull request is ready for review: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675)
(https://github.com/bitcoin/bitcoin/pull/29675)
📝 brunoerg opened a pull request: "fuzz: fix bad alloc in connman target"
(https://github.com/bitcoin/bitcoin/pull/31235)
Fixes #31234
This PR limites the value of `max_addresses` and `max_pct` (chose same values used in the addrman target). Otherwise, we can have memory issues (e.g. vector allocation).
(https://github.com/bitcoin/bitcoin/pull/31235)
Fixes #31234
This PR limites the value of `max_addresses` and `max_pct` (chose same values used in the addrman target). Otherwise, we can have memory issues (e.g. vector allocation).
📝 secp512k2 opened a pull request: "doc: Add missing 'blank=true' option in offline-signing-tutorial.md"
(https://github.com/bitcoin/bitcoin/pull/31236)
### **Issue:**
The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option.
---
### **Details:**
**Original Text:**
> This is achieved by using the `createwallet` options: `disable_private_keys=true, blank=true`.
**Original Command:**
```sh
[online]$ ./build/src/bitcoin-cli -signet -named createwallet \
wall
...
(https://github.com/bitcoin/bitcoin/pull/31236)
### **Issue:**
The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option.
---
### **Details:**
**Original Text:**
> This is achieved by using the `createwallet` options: `disable_private_keys=true, blank=true`.
**Original Command:**
```sh
[online]$ ./build/src/bitcoin-cli -signet -named createwallet \
wall
...
💬 maflcko commented on pull request "doc: Add missing 'blank=true' option in offline-signing-tutorial.md":
(https://github.com/bitcoin/bitcoin/pull/31236#issuecomment-2460466928)
Please don't include the full diff in the pull request description. If someone wanted to look at the diff, they could just do so directly. Also, the diff in the pull description is different from the actual diff, which is broken (invalid syntax).
(https://github.com/bitcoin/bitcoin/pull/31236#issuecomment-2460466928)
Please don't include the full diff in the pull request description. If someone wanted to look at the diff, they could just do so directly. Also, the diff in the pull description is different from the actual diff, which is broken (invalid syntax).
🤔 maflcko reviewed a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2412663196)
left some nits on the first commit. Will take a look at the others later.
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2412663196)
left some nits on the first commit. Will take a look at the others later.
💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1827490394)
Any reason to log the file name here? The containing folder of the log should already have the name of the test/file. If not, the user should know which test they called to know it.
If not, and this was helpful, it would be better to log it for all tests, instead of just for this one test specifically.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1827490394)
Any reason to log the file name here? The containing folder of the log should already have the name of the test/file. If not, the user should know which test they called to know it.
If not, and this was helpful, it would be better to log it for all tests, instead of just for this one test specifically.