💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2140656559)
> > I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4n higher than before, and stuck looking to mine a first block at full difficulty.
>
> I don't think that's much of a concern -- all you'd need to do is invalidateblock the last block of the period, mine a new on
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2140656559)
> > I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4n higher than before, and stuck looking to mine a first block at full difficulty.
>
> I don't think that's much of a concern -- all you'd need to do is invalidateblock the last block of the period, mine a new on
...
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621295595)
Taken, thanks!
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621295595)
Taken, thanks!
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621295778)
Ok, passing CChain and BlockMan as parameters now.
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621295778)
Ok, passing CChain and BlockMan as parameters now.
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621295907)
Taken, thanks!
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621295907)
Taken, thanks!
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621298451)
I have adopted these changes with minor edits and that addresses your previous comment about the genesis behavior as well.
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621298451)
I have adopted these changes with minor edits and that addresses your previous comment about the genesis behavior as well.
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621299243)
Addressed in the other comment on this commit
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1621299243)
Addressed in the other comment on this commit
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2140704680)
Addressed comments from @ryanofsky , thank you for the thoughtful review!
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2140704680)
Addressed comments from @ryanofsky , thank you for the thoughtful review!
📝 mzumsande opened a pull request: "validation: Improve, document and test logic for chains building on invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/30207)
Some improvements with respect to invalid blocks and blocks building on them:
1.) The fields `m_best_header` (best header irrespective on whether we have the full block or not) and `BLOCK_FAILED_CHILD` (BlockStatus saying that a header descends from an invalid block) are populated on a best-effort basis and can sometimes be off. This is not too bad because critical consensus code doesn't rely on either of these, but it is kind of brittle, especially because this is not clearly documented. Do
...
(https://github.com/bitcoin/bitcoin/pull/30207)
Some improvements with respect to invalid blocks and blocks building on them:
1.) The fields `m_best_header` (best header irrespective on whether we have the full block or not) and `BLOCK_FAILED_CHILD` (BlockStatus saying that a header descends from an invalid block) are populated on a best-effort basis and can sometimes be off. This is not too bad because critical consensus code doesn't rely on either of these, but it is kind of brittle, especially because this is not clearly documented. Do
...
🤔 furszy reviewed a pull request: "validation: Improve, document and test logic for chains building on invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/30207#pullrequestreview-2089313850)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30207#pullrequestreview-2089313850)
Concept ACK
💬 TheCharlatan commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2140862734)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2140862734)
Concept ACK
💬 naiyoma commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1621428596)
Thanks for pointing this out, the reference specifies an implicit conversion from bool to int is unsafe for `T::operator bool() const;`, but for this scenario, since we are not using a custom class it could be safe
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1621428596)
Thanks for pointing this out, the reference specifies an implicit conversion from bool to int is unsafe for `T::operator bool() const;`, but for this scenario, since we are not using a custom class it could be safe
💬 naiyoma commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1621439305)
I wasn't keen on choosing between `IsArgSet() `and `GetBoolArg() `since both achieve the same desired outcome, but you are certainly right; using either of the two, for all would be a better approach.
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1621439305)
I wasn't keen on choosing between `IsArgSet() `and `GetBoolArg() `since both achieve the same desired outcome, but you are certainly right; using either of the two, for all would be a better approach.
💬 maflcko commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1621447372)
No, they are not the same. One checks whether the arg is set at all (it could be set to 0), the other checks if the arg is set to 1.
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1621447372)
No, they are not the same. One checks whether the arg is set at all (it could be set to 0), the other checks if the arg is set to 1.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621482991)
What is done here is a weight-to-vsize conversion, like also done in e.g. `CTransaction.get_vsize()`, so it's essentially dividing by 4 and rounding up. Added a comment to clarify.
> Tying this to an earlier comment of mine - if target_weight had been called target_weight_wu, the code would become more self explanatory.
To my knowledge, weight is always measured in weight units, so I don't think adding the `_wu` suffix here would add any new information.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621482991)
What is done here is a weight-to-vsize conversion, like also done in e.g. `CTransaction.get_vsize()`, so it's essentially dividing by 4 and rounding up. Added a comment to clarify.
> Tying this to an earlier comment of mine - if target_weight had been called target_weight_wu, the code would become more self explanatory.
To my knowledge, weight is always measured in weight units, so I don't think adding the `_wu` suffix here would add any new information.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483053)
Good idea. Leaving that as a follow-up, as the introduction of that constant could likely be used in much more places than only the ones I touch (maybe even other functional tests).
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483053)
Good idea. Leaving that as a follow-up, as the introduction of that constant could likely be used in much more places than only the ones I touch (maybe even other functional tests).
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483090)
Agree, leaving that as a follow-up as it likely needs more instances to adapt that are not touched here.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483090)
Agree, leaving that as a follow-up as it likely needs more instances to adapt that are not touched here.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483152)
> I've been trying to understand why 1 is subtracted here, but could not come up with a proper reason. Is it somehow related to excluding the already serialised length of the output script before padding?
Yes, exactly. The encoded length of the unpadded output script fits into one byte, and we want to find how much the length encoding increases with the padding, so we subtract by one. I added a comment to (hopefully) reduce the confusion for code readers.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483152)
> I've been trying to understand why 1 is subtracted here, but could not come up with a proper reason. Is it somehow related to excluding the already serialised length of the output script before padding?
Yes, exactly. The encoded length of the unpadded output script fits into one byte, and we want to find how much the length encoding increases with the padding, so we subtract by one. I added a comment to (hopefully) reduce the confusion for code readers.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483290)
> Why are you incrementing by 3 twice
The first increment by 3 is done to find the worst-case weight (that could result from ._bulk_tx), the second is part of a weight-to-vsize conversion. I've changed the latter to use float division and rounding up, as that should reflect better what this calculation actually achieves (the same way it's done e.g. in `CTransaction.get_vsize()`). It might be worth to introduce a `weight_to_vsize` helper in the util module in a follow-up, which could probably
...
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483290)
> Why are you incrementing by 3 twice
The first increment by 3 is done to find the worst-case weight (that could result from ._bulk_tx), the second is part of a weight-to-vsize conversion. I've changed the latter to use float division and rounding up, as that should reflect better what this calculation actually achieves (the same way it's done e.g. in `CTransaction.get_vsize()`). It might be worth to introduce a `weight_to_vsize` helper in the util module in a follow-up, which could probably
...
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483453)
This sounds like a very good idea for a follow-up, as it's not directly related to this PR (though one could argue that it's more likely to happen with large `target_weight` values). I think it's even worth it to throw an exception with an explicit message here like "UTXO is too small to cover for fees" (maybe even including details like utxo value, absolute fee, fee-rate, vsize) rather than only an assertion on the send value, which could still be confusing for users.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483453)
This sounds like a very good idea for a follow-up, as it's not directly related to this PR (though one could argue that it's more likely to happen with large `target_weight` values). I think it's even worth it to throw an exception with an explicit message here like "UTXO is too small to cover for fees" (maybe even including details like utxo value, absolute fee, fee-rate, vsize) rather than only an assertion on the send value, which could still be confusing for users.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#issuecomment-2140953559)
@rkrux @ismaelsadeeq Thanks for your reviews, much appreciated! I tried to tackle the comments, though for some of them I suggested that they would be a better fit for a follow-up PR, to not increase the size this PR (focused on functional change) with too many refactoring commits that would likely make most sense with changes also in other functional tests and framework modules.
(https://github.com/bitcoin/bitcoin/pull/30162#issuecomment-2140953559)
@rkrux @ismaelsadeeq Thanks for your reviews, much appreciated! I tried to tackle the comments, though for some of them I suggested that they would be a better fit for a follow-up PR, to not increase the size this PR (focused on functional change) with too many refactoring commits that would likely make most sense with changes also in other functional tests and framework modules.