💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598234205)
I did an [analysis](https://github.com/setavenger/BIP0352-light-client-specification?tab=readme-ov-file#dust-limits) a while back which revealed that 10% of taproot UTXOs are <= 330. I'm not sure about the details but I believe it was the entire UTXO set collected by the indexer up to 834761. At the time it collected the entire set and not only SP-eligible UTXOs.
I guess it's technically possible that either none of those 10% were in eligible transactions and/or they were combined with a bigg
...
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598234205)
I did an [analysis](https://github.com/setavenger/BIP0352-light-client-specification?tab=readme-ov-file#dust-limits) a while back which revealed that 10% of taproot UTXOs are <= 330. I'm not sure about the details but I believe it was the entire UTXO set collected by the indexer up to 834761. At the time it collected the entire set and not only SP-eligible UTXOs.
I guess it's technically possible that either none of those 10% were in eligible transactions and/or they were combined with a bigg
...
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2107197805)
@paplorinc I noticed you approved the PR, but we don't really use github's approval/review flow for determining the review status of a PR.
Can you instead leave an `ACK <commit>` message per https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review ? This way, intent is clear to the maintainers and DrahtBot can track the status and update the summary comment.
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2107197805)
@paplorinc I noticed you approved the PR, but we don't really use github's approval/review flow for determining the review status of a PR.
Can you instead leave an `ACK <commit>` message per https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review ? This way, intent is clear to the maintainers and DrahtBot can track the status and update the summary comment.
🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-2050218566)
some test review
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-2050218566)
some test review
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1596843725)
I don't really understand what's being tested here - this doesn't have anything to do with package RBF? This isn't a topologically valid package since the transactions aren't related to each other in addition to being double spends. #30066 seems closer to what this is going for?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1596843725)
I don't really understand what's being tested here - this doesn't have anything to do with package RBF? This isn't a topologically valid package since the transactions aren't related to each other in addition to being double spends. #30066 seems closer to what this is going for?
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1596837120)
nit: you change the fee in the last successful one, which is a little bit sus since fee shouldn't matter in this test
Maybe create 2 constants for the parent `fee_per_output` and child fee/feerate, and use them for each round?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1596837120)
nit: you change the fee in the last successful one, which is a little bit sus since fee shouldn't matter in this test
Maybe create 2 constants for the parent `fee_per_output` and child fee/feerate, and use them for each round?
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598149576)
nit: this would be easier to read and more extensible in the future
```suggestion
const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize);
```
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598149576)
nit: this would be easier to read and more extensible in the future
```suggestion
const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize);
```
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1596847344)
These 2 things are contradictory. AFAICT `fee_rate` is ignored, delete?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1596847344)
These 2 things are contradictory. AFAICT `fee_rate` is ignored, delete?
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598137245)
This is now in `mempool_util` (= linter complaint)
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598137245)
This is now in `mempool_util` (= linter complaint)
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598246153)
with rebase
```suggestion
fill_mempool(self, self.nodes[0])
```
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598246153)
with rebase
```suggestion
fill_mempool(self, self.nodes[0])
```
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598211388)
Given that `fee_delta` is a multiple of `DEFAULT_FEE` and you're using `fee_delta / 2` here... I think the general readability of this test would be improved if you created a constant base unit `FEE` which was equal to, say, `DEFAULT_FEE / 10` (or `104 * 10` which would give most of the transactions whole number feerates), and then used `FEE`, `2*FEE`, `8*FEE`, etc. in all the tests.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598211388)
Given that `fee_delta` is a multiple of `DEFAULT_FEE` and you're using `fee_delta / 2` here... I think the general readability of this test would be improved if you created a constant base unit `FEE` which was equal to, say, `DEFAULT_FEE / 10` (or `104 * 10` which would give most of the transactions whole number feerates), and then used `FEE`, `2*FEE`, `8*FEE`, etc. in all the tests.
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598248191)
Maybe there should be some v3 test coverage (including nice 0fee parent package RBF)?
```
diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 8285b82c19..990d36359e 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -579,6 +579,32 @@ class MempoolAcceptV3(BitcoinTestFramework):
)
self.check_mempool([tx_with_multi_children["txid"], tx_with_sibling3_rbf["txid"], tx_with_sibling2["txid"
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598248191)
Maybe there should be some v3 test coverage (including nice 0fee parent package RBF)?
```
diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 8285b82c19..990d36359e 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -579,6 +579,32 @@ class MempoolAcceptV3(BitcoinTestFramework):
)
self.check_mempool([tx_with_multi_children["txid"], tx_with_sibling3_rbf["txid"], tx_with_sibling2["txid"
...
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598170246)
Sorry to nit my own previous suggestion, but I think "parent paying for child replacement" might still be too specific, since it could be that the child has nothing to replace but is lower feerate (like `package_txns6` in the functional test).
Maybe
```suggestion
"package RBF failed: package feerate is less than parent feerate",
strprintf("package feerate %s <= parent feerate %s", package_feerate.ToString(), parent_f
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598170246)
Sorry to nit my own previous suggestion, but I think "parent paying for child replacement" might still be too specific, since it could be that the child has nothing to replace but is lower feerate (like `package_txns6` in the functional test).
Maybe
```suggestion
"package RBF failed: package feerate is less than parent feerate",
strprintf("package feerate %s <= parent feerate %s", package_feerate.ToString(), parent_f
...
💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2107220524)
ACK d196442c015c4cbf490751a650ecb3aa29321442
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2107220524)
ACK d196442c015c4cbf490751a650ecb3aa29321442
✅ maflcko closed an issue: "~/.bitcoin (which is a softlink to a separate vmware virtual drive) dir is now almost 1tb"
(https://github.com/bitcoin/bitcoin/issues/29909)
(https://github.com/bitcoin/bitcoin/issues/29909)
💬 maflcko commented on issue "~/.bitcoin (which is a softlink to a separate vmware virtual drive) dir is now almost 1tb":
(https://github.com/bitcoin/bitcoin/issues/29909#issuecomment-2107249656)
Closing for now, due to inactivity, but the discussion can be continued regardless.
(https://github.com/bitcoin/bitcoin/issues/29909#issuecomment-2107249656)
Closing for now, due to inactivity, but the discussion can be continued regardless.
💬 maflcko commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2107257666)
> I have installed them but turned --with-gui=no in the configure option
Do you want the gui? If not, then disabling is the right choice, but then the gui unit tests shouldn't fail, because they shouldn't exist. If they do, that is a bug.
Again, there is little that can be done here, unless you share more details on the steps to reproduce from the book. See https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2090021205
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2107257666)
> I have installed them but turned --with-gui=no in the configure option
Do you want the gui? If not, then disabling is the right choice, but then the gui unit tests shouldn't fail, because they shouldn't exist. If they do, that is a bug.
Again, there is little that can be done here, unless you share more details on the steps to reproduce from the book. See https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2090021205
✅ maflcko closed an issue: "Bitcoin Core 27 crash at sync - Ubuntu - No error in logs"
(https://github.com/bitcoin/bitcoin/issues/30001)
(https://github.com/bitcoin/bitcoin/issues/30001)
💬 maflcko commented on issue "Bitcoin Core 27 crash at sync - Ubuntu - No error in logs":
(https://github.com/bitcoin/bitcoin/issues/30001#issuecomment-2107260441)
Closing for now due to inactivity, but please leave a comment if you have more details or information.
(https://github.com/bitcoin/bitcoin/issues/30001#issuecomment-2107260441)
Closing for now due to inactivity, but please leave a comment if you have more details or information.
💬 maflcko commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2107333523)
utACK 04ffe4061da2d0135f206032e167703772b5da78 🎪
<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: utACK 04ffe4061da2d0135f2060
...
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2107333523)
utACK 04ffe4061da2d0135f206032e167703772b5da78 🎪
<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: utACK 04ffe4061da2d0135f2060
...
💬 fanquake commented on pull request "contrib: use ENV flags in get_arch":
(https://github.com/bitcoin/bitcoin/pull/30074#issuecomment-2107335721)
Guix Build (aarch64):
```bash
fcb75f2a3b61befeabe143301e5db490d23b1ee4a5edb109f219dea575395b49 guix-build-b59a027d957a/output/aarch64-linux-gnu/SHA256SUMS.part
15310e3aff2a9ef71bad06315fcd425e56611d5bed7f6c394f3fe2248140e17b guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu-debug.tar.gz
8e60146d39a47e9c332e3b842e239f1ca7773ca2e6788876155377f4568ab1fa guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu.tar.gz
104322
...
(https://github.com/bitcoin/bitcoin/pull/30074#issuecomment-2107335721)
Guix Build (aarch64):
```bash
fcb75f2a3b61befeabe143301e5db490d23b1ee4a5edb109f219dea575395b49 guix-build-b59a027d957a/output/aarch64-linux-gnu/SHA256SUMS.part
15310e3aff2a9ef71bad06315fcd425e56611d5bed7f6c394f3fe2248140e17b guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu-debug.tar.gz
8e60146d39a47e9c332e3b842e239f1ca7773ca2e6788876155377f4568ab1fa guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu.tar.gz
104322
...