💬 maflcko commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1441426330)
I presume `self.def_wallet` is not equal to `self.wallet`, so a mempool sync is missing here, to allow for the transaction to "propagate" through the full validation interface queue.
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1441426330)
I presume `self.def_wallet` is not equal to `self.wallet`, so a mempool sync is missing here, to allow for the transaction to "propagate" through the full validation interface queue.
🤔 stratospher reviewed a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1803647850)
tested ACK fb5bfed. addrfetch + manual connections aren't frequent and it would be useful to have this for transition to v2 one day.
used `-connect`, `-addnode`, `-seednode` and observed reconnections happening if other peer was v1 and no reconnection if other peer was v2. an interesting behaviour which i realised made sense was to keep retrying with v2 when latency issue failures happen on tor.
```
2024-01-04T06:14:58Z [net:debug] trying v2 connection "xyz" lastseen=0.0hrs
2024-01-04
...
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1803647850)
tested ACK fb5bfed. addrfetch + manual connections aren't frequent and it would be useful to have this for transition to v2 one day.
used `-connect`, `-addnode`, `-seednode` and observed reconnections happening if other peer was v1 and no reconnection if other peer was v2. an interesting behaviour which i realised made sense was to keep retrying with v2 when latency issue failures happen on tor.
```
2024-01-04T06:14:58Z [net:debug] trying v2 connection "xyz" lastseen=0.0hrs
2024-01-04
...
📝 Retropex opened a pull request: "doc: revert clarify -datacarriersize"
(https://github.com/bitcoin/bitcoin/pull/29173)
The latest update of the help text of `-datacarriersize` is incorrect.
The purpose of this standardization rule is not to target only the data contained in the raw scriptPubKey, but all forms of arbitrary data.
It is incorrect to change the description of this option if an attempt to update it was made without being merged.
Context:
The [first inscription](https://mempool.space/block/000000000000000000029730547464f056f8b6e2e0a02eaf69c24389983a04f5) appeared December 14, 2022 at the h
...
(https://github.com/bitcoin/bitcoin/pull/29173)
The latest update of the help text of `-datacarriersize` is incorrect.
The purpose of this standardization rule is not to target only the data contained in the raw scriptPubKey, but all forms of arbitrary data.
It is incorrect to change the description of this option if an attempt to update it was made without being merged.
Context:
The [first inscription](https://mempool.space/block/000000000000000000029730547464f056f8b6e2e0a02eaf69c24389983a04f5) appeared December 14, 2022 at the h
...
💬 ChrisMartl commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876707992)
ACK
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876707992)
ACK
💬 ajtowns commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876721138)
NACK. Docs should describe what the code does, not what we might like it to do.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876721138)
NACK. Docs should describe what the code does, not what we might like it to do.
💬 maflcko commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876721749)
> Then a [second patch](https://github.com/bitcoin/bitcoin/pull/28408), this time eligible for a merge, was released on September 5, 2023
>
> Finally @maflcko changed the description on June 8, 2023.
I changed the documentation to match the code in June. Otherwise, there would be a mismatch between the code and the documentation. I am not sure why it would be beneficial to re-introduce the mismatch.
I have no objection to changing the code (along with the documentation). However, I thin
...
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876721749)
> Then a [second patch](https://github.com/bitcoin/bitcoin/pull/28408), this time eligible for a merge, was released on September 5, 2023
>
> Finally @maflcko changed the description on June 8, 2023.
I changed the documentation to match the code in June. Otherwise, there would be a mismatch between the code and the documentation. I am not sure why it would be beneficial to re-introduce the mismatch.
I have no objection to changing the code (along with the documentation). However, I thin
...
💬 t-bast commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876742224)
> No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since you're advocating for removing the CPFP part! I guess we're just talking
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876742224)
> No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since you're advocating for removing the CPFP part! I guess we're just talking
...
💬 maflcko commented on pull request "fuzz: set `nMaxOutboundLimit` in connman target":
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1876742834)
lgtm ACK 46d7113ec1389eb78d7cd44425ecc22dda9b67bf
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1876742834)
lgtm ACK 46d7113ec1389eb78d7cd44425ecc22dda9b67bf
💬 1ma commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876752317)
ACK. The change that is being reverted was a covert attempt at shrugging off the responsibility of addressing [CVE-2023-50428](https://nvd.nist.gov/vuln/detail/CVE-2023-50428) in Bitcoin Core.
There is even an open PR to address it, https://github.com/bitcoin/bitcoin/pull/28408. Exploits must be addressed by changing the C++ code, not the project's documentation.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876752317)
ACK. The change that is being reverted was a covert attempt at shrugging off the responsibility of addressing [CVE-2023-50428](https://nvd.nist.gov/vuln/detail/CVE-2023-50428) in Bitcoin Core.
There is even an open PR to address it, https://github.com/bitcoin/bitcoin/pull/28408. Exploits must be addressed by changing the C++ code, not the project's documentation.
💬 maflcko commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876780967)
> ACK. The change that is being reverted was a covert attempt at shrugging off the responsibility of addressing [CVE-2023-50428](https://nvd.nist.gov/vuln/detail/CVE-2023-50428) in Bitcoin Core.
> There is even an open PR to address it, #28408. Exploits must be addressed by changing the C++ code, not the project's documentation.
This PR was opened and the CVE was filed *after* the documentation change, so it is not possible that the documentation change was made in response to either of thos
...
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876780967)
> ACK. The change that is being reverted was a covert attempt at shrugging off the responsibility of addressing [CVE-2023-50428](https://nvd.nist.gov/vuln/detail/CVE-2023-50428) in Bitcoin Core.
> There is even an open PR to address it, #28408. Exploits must be addressed by changing the C++ code, not the project's documentation.
This PR was opened and the CVE was filed *after* the documentation change, so it is not possible that the documentation change was made in response to either of thos
...
💬 TheCharlatan commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876805028)
NACK, this re-introduces an inconsistency.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876805028)
NACK, this re-introduces an inconsistency.
⚠️ hebasto opened an issue: "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`"
(https://github.com/bitcoin/bitcoin/issues/29174)
Apple Clang 15 does not support `-Xclang -internal-isystem` anymore. On both `x86_64` and `arm64` platforms, the following check: https://github.com/bitcoin/bitcoin/blob/65c05db660b2ca1d0076b0d8573a6760b3228068/configure.ac#L742-L744
fails with `error: unknown argument: '-internal-isystem/usr/local/include'`.
It makes the `--enable-suppress-external-warnings` option not working for dependency packages installed by Homebrew and linked to `/usr/local`, which happens for all non-keg packages o
...
(https://github.com/bitcoin/bitcoin/issues/29174)
Apple Clang 15 does not support `-Xclang -internal-isystem` anymore. On both `x86_64` and `arm64` platforms, the following check: https://github.com/bitcoin/bitcoin/blob/65c05db660b2ca1d0076b0d8573a6760b3228068/configure.ac#L742-L744
fails with `error: unknown argument: '-internal-isystem/usr/local/include'`.
It makes the `--enable-suppress-external-warnings` option not working for dependency packages installed by Homebrew and linked to `/usr/local`, which happens for all non-keg packages o
...
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441549930)
> Again, this should affect everyone on macOS today already on current master, using clang-15 (or later), so a separate issue and bugfix can be filed?
See https://github.com/bitcoin/bitcoin/issues/29174.
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441549930)
> Again, this should affect everyone on macOS today already on current master, using clang-15 (or later), so a separate issue and bugfix can be filed?
See https://github.com/bitcoin/bitcoin/issues/29174.
💬 brunoerg commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876817939)
Concept NACK.
> The purpose of this standardization rule is not to target only the data contained in the raw scriptPubKey, but all forms of arbitrary data.
This is not what it currently does.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876817939)
Concept NACK.
> The purpose of this standardization rule is not to target only the data contained in the raw scriptPubKey, but all forms of arbitrary data.
This is not what it currently does.
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441554975)
Thanks, resolving discussion here for now
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441554975)
Thanks, resolving discussion here for now
💬 sp-marcel-hernandez commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876819882)
@TheCharlatan @brunoerg your concerns are easily addressed by reviewing and merging the PR for the actual bugfix: https://github.com/bitcoin/bitcoin/pull/28408
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876819882)
@TheCharlatan @brunoerg your concerns are easily addressed by reviewing and merging the PR for the actual bugfix: https://github.com/bitcoin/bitcoin/pull/28408
💬 Sun0fABeach commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876832867)
ACK. Good job catching a developer trying to sweep an important issue under the rug. Very concerning tbh.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876832867)
ACK. Good job catching a developer trying to sweep an important issue under the rug. Very concerning tbh.
✅ glozow closed a pull request: "doc: revert clarify -datacarriersize"
(https://github.com/bitcoin/bitcoin/pull/29173)
(https://github.com/bitcoin/bitcoin/pull/29173)
💬 glozow commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876847952)
NACK, the current documentation is more accurate to what the code actually does.
If you want to change what the code does, that's already its own PR #28408.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876847952)
NACK, the current documentation is more accurate to what the code actually does.
If you want to change what the code does, that's already its own PR #28408.
📝 torkelrogstad opened a pull request: "rpc: validate fee estimation mode case insensitive"
(https://github.com/bitcoin/bitcoin/pull/29175)
The RPC docs for `send` claim that `estimate_mode` is case insensitive. This is not the case on master:
https://github.com/bitcoin/bitcoin/blob/65c05db660b2ca1d0076b0d8573a6760b3228068/src/wallet/rpc/spend.cpp#L193-L195
This PR fixes that, by converting the estimation mode to lower case before comparing to `"unset"`. Note that the actual _parsing_ of the estimation mode already happens case insensitively, further down in the same function.
(https://github.com/bitcoin/bitcoin/pull/29175)
The RPC docs for `send` claim that `estimate_mode` is case insensitive. This is not the case on master:
https://github.com/bitcoin/bitcoin/blob/65c05db660b2ca1d0076b0d8573a6760b3228068/src/wallet/rpc/spend.cpp#L193-L195
This PR fixes that, by converting the estimation mode to lower case before comparing to `"unset"`. Note that the actual _parsing_ of the estimation mode already happens case insensitively, further down in the same function.
👍 TheCharlatan approved a pull request: "fuzz: rule-out too deep derivation paths in descriptor parsing targets"
(https://github.com/bitcoin/bitcoin/pull/28832#pullrequestreview-1803919066)
ACK a44808fb437864878c2d9696b8a96193091446ee
(https://github.com/bitcoin/bitcoin/pull/28832#pullrequestreview-1803919066)
ACK a44808fb437864878c2d9696b8a96193091446ee