💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683935476)
> Well, it will be removed, given that someone writes the code and reviews it.
A partial fix is in https://github.com/bitcoin/bitcoin/pull/30482. It will be continued in the future.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683935476)
> Well, it will be removed, given that someone writes the code and reviews it.
A partial fix is in https://github.com/bitcoin/bitcoin/pull/30482. It will be continued in the future.
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2238562006)
re-ACK b4dd7ab43e8cfc2c171f67588e4e1ec2705393c2 🕴
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK b4dd7ab43e8cfc2c171f
...
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2238562006)
re-ACK b4dd7ab43e8cfc2c171f67588e4e1ec2705393c2 🕴
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK b4dd7ab43e8cfc2c171f
...
💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683976467)
> it could iterate past the end of the of `string_view` and read uninitialized memory.
Right, because on master we rely on stopping when `HexDigit()` returns a `-1`, which is fine when the string is null-terminated but is not safe otherwise. Thanks for pointing this out, this also clarifies my misunderstanding about your [earlier comment](https://github.com/bitcoin/bitcoin/pull/30436/#pullrequestreview-2186195988).
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683976467)
> it could iterate past the end of the of `string_view` and read uninitialized memory.
Right, because on master we rely on stopping when `HexDigit()` returns a `-1`, which is fine when the string is null-terminated but is not safe otherwise. Thanks for pointing this out, this also clarifies my misunderstanding about your [earlier comment](https://github.com/bitcoin/bitcoin/pull/30436/#pullrequestreview-2186195988).
⚠️ Arroz01 opened an issue: "New"
(https://github.com/bitcoin/bitcoin/issues/30484)
(https://github.com/bitcoin/bitcoin/issues/30484)
💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1683988688)
> In other places, `os.path.realpath(__file__)` is used in conjunction with relative paths in the `test/functional/` directory.
Thank you for the explanation. It makes sense, given that cmake creates all links recursively in this dir.
However, this leads back to my original question: Why not change the other affected places?
This still fails for me (and I expect it to fail in the cmake branch as well):
```
$ ln $PWD/../test/functional/interface_rest.py ./test/functional/
$
...
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1683988688)
> In other places, `os.path.realpath(__file__)` is used in conjunction with relative paths in the `test/functional/` directory.
Thank you for the explanation. It makes sense, given that cmake creates all links recursively in this dir.
However, this leads back to my original question: Why not change the other affected places?
This still fails for me (and I expect it to fail in the cmake branch as well):
```
$ ln $PWD/../test/functional/interface_rest.py ./test/functional/
$
...
✅ maflcko closed an issue: "New"
(https://github.com/bitcoin/bitcoin/issues/30484)
(https://github.com/bitcoin/bitcoin/issues/30484)
:lock: fanquake locked an issue: "New"
(https://github.com/bitcoin/bitcoin/issues/30484)
(https://github.com/bitcoin/bitcoin/issues/30484)
💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2238694916)
> Do you disagree that it is harmless, or do you disagree that the same problem exists in a broadcast pool?
Both. A broadcast pool has direct access to policy rules, a wallet client generally does not.
> Also, why would a fee bump be unnecessary when the transaction doesn't even meet the min mempool fee?
Because it may still exist in a larger miner's mempool. Consider the following situation: Alice broadcasts a transaction at a fee rate of 10 sats/vb. Her min mempool fee increases late
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2238694916)
> Do you disagree that it is harmless, or do you disagree that the same problem exists in a broadcast pool?
Both. A broadcast pool has direct access to policy rules, a wallet client generally does not.
> Also, why would a fee bump be unnecessary when the transaction doesn't even meet the min mempool fee?
Because it may still exist in a larger miner's mempool. Consider the following situation: Alice broadcasts a transaction at a fee rate of 10 sats/vb. Her min mempool fee increases late
...
💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2238698096)
Added `bool& tip_changed`.
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2238698096)
Added `bool& tip_changed`.
🚀 fanquake merged a pull request: "depends: build FreeType with CMake"
(https://github.com/bitcoin/bitcoin/pull/29880)
(https://github.com/bitcoin/bitcoin/pull/29880)
💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2238702885)
My Guix build:
```
x86_64
5cc76ca8f123e7ff2a365d6a94246a949c4976cf9ecfb9056a4e696a1b80b097 guix-build-e096f5d456f7/output/aarch64-linux-gnu/SHA256SUMS.part
1c6c8d2efba8f6cbe54b8e8bde00d962f9837ab76f801507a370e4d15ada0059 guix-build-e096f5d456f7/output/aarch64-linux-gnu/bitcoin-e096f5d456f7-aarch64-linux-gnu-debug.tar.gz
27110e79b03a3f22c24888f79e84c684eaddfb04b8595d200d6db5d689f260a2 guix-build-e096f5d456f7/output/aarch64-linux-gnu/bitcoin-e096f5d456f7-aarch64-linux-gnu.tar.gz
ddb3c16f6
...
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2238702885)
My Guix build:
```
x86_64
5cc76ca8f123e7ff2a365d6a94246a949c4976cf9ecfb9056a4e696a1b80b097 guix-build-e096f5d456f7/output/aarch64-linux-gnu/SHA256SUMS.part
1c6c8d2efba8f6cbe54b8e8bde00d962f9837ab76f801507a370e4d15ada0059 guix-build-e096f5d456f7/output/aarch64-linux-gnu/bitcoin-e096f5d456f7-aarch64-linux-gnu-debug.tar.gz
27110e79b03a3f22c24888f79e84c684eaddfb04b8595d200d6db5d689f260a2 guix-build-e096f5d456f7/output/aarch64-linux-gnu/bitcoin-e096f5d456f7-aarch64-linux-gnu.tar.gz
ddb3c16f6
...
🤔 glozow reviewed a pull request: "fuzz: Speed up PickValue in txorphan"
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2186411295)
concept ACK, thanks @maflcko!
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2186411295)
concept ACK, thanks @maflcko!
💬 glozow commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1683198029)
Agree that using a vector (i.e. the 3-line diff) does seem like it solves this problem in a simpler + faster way. Having duplicate outpoints should be fine and maybe even more coverage. Fuzzed with the 3-line diff for a bit.
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1683198029)
Agree that using a vector (i.e. the 3-line diff) does seem like it solves this problem in a simpler + faster way. Having duplicate outpoints should be fine and maybe even more coverage. Fuzzed with the 3-line diff for a bit.
💬 fanquake commented on issue "guix: build multiprocess":
(https://github.com/bitcoin/bitcoin/issues/30483#issuecomment-2238748982)
> That didn't work because curl is not accessible.
Can you elaborate? I did a build with `MULTIPROCESS=1` (https://github.com/fanquake/bitcoin/tree/guix_multiprocess) and it worked fine:
```bash
# time BASE_CACHE="/base_cache" SOURCES_PATH="/sources" SDK_PATH="/SDKs" HOSTS="x86_64-linux-gnu" ./contrib/guix/guix-build
e4e606e3a7076fe0e1f93b98ecdfbbfcd552a3c25f368684f3e94aab919058ce guix-build-e0e38abd03a0/output/dist-archive/bitcoin-e0e38abd03a0.tar.gz
e8b4c9f20abfe29fa1f0fa47515a28d169f
...
(https://github.com/bitcoin/bitcoin/issues/30483#issuecomment-2238748982)
> That didn't work because curl is not accessible.
Can you elaborate? I did a build with `MULTIPROCESS=1` (https://github.com/fanquake/bitcoin/tree/guix_multiprocess) and it worked fine:
```bash
# time BASE_CACHE="/base_cache" SOURCES_PATH="/sources" SDK_PATH="/SDKs" HOSTS="x86_64-linux-gnu" ./contrib/guix/guix-build
e4e606e3a7076fe0e1f93b98ecdfbbfcd552a3c25f368684f3e94aab919058ce guix-build-e0e38abd03a0/output/dist-archive/bitcoin-e0e38abd03a0.tar.gz
e8b4c9f20abfe29fa1f0fa47515a28d169f
...
👍 t-bast approved a pull request: "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending"
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2187787797)
ACK https://github.com/bitcoin/bitcoin/pull/30352/commits/8d956ccb86d70e2059a5e2ccd8e63b7f7c8e3842, thanks for pushing this!
I've read through the code and comments on this PR, and to me it makes sense to use a taproot 2-byte program, the cute trick of having it encode as "fees" in bech32m can be useful (since we need to arbitrarily choose 2 bytes anyway for the program), and it's a step towards ephemeral anchors which are much better than current anchor outputs :+1:
Regarding utxo bloat
...
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2187787797)
ACK https://github.com/bitcoin/bitcoin/pull/30352/commits/8d956ccb86d70e2059a5e2ccd8e63b7f7c8e3842, thanks for pushing this!
I've read through the code and comments on this PR, and to me it makes sense to use a taproot 2-byte program, the cute trick of having it encode as "fees" in bech32m can be useful (since we need to arbitrarily choose 2 bytes anyway for the program), and it's a step towards ephemeral anchors which are much better than current anchor outputs :+1:
Regarding utxo bloat
...
💬 t-bast commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2238752561)
Concept ACK :+1:
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2238752561)
Concept ACK :+1:
💬 fanquake commented on pull request "Stratum v2 Template Provider common functionality":
(https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2238759084)
Can you clarify what the point of PRs like this is, given you have a number of them open? They aren't reviewable, or mergable (given they are all based on various PRs, which them selves are in various stages of draft/WIP), and the continual pushes keep taking up all the CI resources in the repo (I thought we'd adjusted the CI so that you can use your own repo?).
(https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2238759084)
Can you clarify what the point of PRs like this is, given you have a number of them open? They aren't reviewable, or mergable (given they are all based on various PRs, which them selves are in various stages of draft/WIP), and the continual pushes keep taking up all the CI resources in the repo (I thought we'd adjusted the CI so that you can use your own repo?).
💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2238799112)
The same problem exists in a broadcast pool. For example:
* Alice broadcasts at 10sat/vb, but the transaction isn't confirmed in $mempool_expiry time, and thus dropped from Alice's mempool (and broadcast pool). (The transaction may or may not be in a miner's mempool with larger expiry)
* Mallory broadcasts her at 9sat/vb.
* Alice's wallet discards her transaction using 10sat/vb, based on the hierarchy `confirmed > mempool > local` (or `confirmed > mempool > broadcast_pool > local`)
So I
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2238799112)
The same problem exists in a broadcast pool. For example:
* Alice broadcasts at 10sat/vb, but the transaction isn't confirmed in $mempool_expiry time, and thus dropped from Alice's mempool (and broadcast pool). (The transaction may or may not be in a miner's mempool with larger expiry)
* Mallory broadcasts her at 9sat/vb.
* Alice's wallet discards her transaction using 10sat/vb, based on the hierarchy `confirmed > mempool > local` (or `confirmed > mempool > broadcast_pool > local`)
So I
...
💬 fanquake commented on pull request "depends: build expat with CMake":
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2238813040)
Rebased on master. 32-bit builds should also be fixed. No longer a version bump, just a Autotools -> CMake switch.
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2238813040)
Rebased on master. 32-bit builds should also be fixed. No longer a version bump, just a Autotools -> CMake switch.
👋 fanquake's pull request is ready for review: "depends: build expat with CMake"
(https://github.com/bitcoin/bitcoin/pull/29878)
(https://github.com/bitcoin/bitcoin/pull/29878)