💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598219770)
Updated.

💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598219971)
Fixed.

💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2107169922)
Updated comment per @paplorinc 's suggestions.

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598222539)
Not super surprising since this is a standardness limit, no? I don't think you can broadcast transactions without the output amount being greater than the dust limit (unless it is an `OP_RETURN`).

💬 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
...

💬 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.

🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(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?

💬 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?

💬 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);
```

💬 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?

💬 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)

💬 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])
```

💬 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.

💬 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"
...

💬 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
...

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(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)

💬 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.

💬 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