👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2192299471)
trACK 9afa2c9e50370b2918377f3f3eac0950a4296ec0
Reviewed the changes in the `key_tests.cpp` since my last review on this PR. Again, all unit and functional including the extended tests pass. Just dropped a name candidate if you consider renaming `KeyPair::data()` and run out of options.
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2192299471)
trACK 9afa2c9e50370b2918377f3f3eac0950a4296ec0
Reviewed the changes in the `key_tests.cpp` since my last review on this PR. Again, all unit and functional including the extended tests pass. Just dropped a name candidate if you consider renaming `KeyPair::data()` and run out of options.
💬 itornaza commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686990120)
If there is not a better alternative, I would suggest `secp256k1_keypair()` as a candidate for renaming this `data()` member function to further signify its use through the cast and be called like so:
`reinterpret_cast<const secp256k1_keypair*>(keypair.secp256k1_keypair())`
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686990120)
If there is not a better alternative, I would suggest `secp256k1_keypair()` as a candidate for renaming this `data()` member function to further signify its use through the cast and be called like so:
`reinterpret_cast<const secp256k1_keypair*>(keypair.secp256k1_keypair())`
💬 murchandamus commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243587592)
Concept ACK
Given the majority of the hashrate using `mempoolfullrbf`, Bitcoin Core should adapt its default behavior to match its expectations to the blocks being published.
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243587592)
Concept ACK
Given the majority of the hashrate using `mempoolfullrbf`, Bitcoin Core should adapt its default behavior to match its expectations to the blocks being published.
💬 murchandamus commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1686996487)
This file is named incorrectly.
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1686996487)
This file is named incorrectly.
👋 naiyoma's pull request is ready for review: "Test/rpc whitelistdefault test"
(https://github.com/bitcoin/bitcoin/pull/29858)
(https://github.com/bitcoin/bitcoin/pull/29858)
💬 1440000bytes commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1687024893)
I can rename it if one of the maintainers can review this pull request because I cannot assign a version that would include this PR.
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1687024893)
I can rename it if one of the maintainers can review this pull request because I cannot assign a version that would include this PR.
💬 naiyoma commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1687028156)
reverted back to `with open(self.nodes[0].datadir_path / "bitcoin.conf", "a", encoding="utf8") as f:`
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1687028156)
reverted back to `with open(self.nodes[0].datadir_path / "bitcoin.conf", "a", encoding="utf8") as f:`
💬 naiyoma commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1687030146)
added
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1687030146)
added
💬 naiyoma commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1687035192)
I've added more descriptive docstrings to each function.
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1687035192)
I've added more descriptive docstrings to each function.
🤔 achow101 reviewed 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-2192397657)
Concept ACK
The code, docs, and the OP sometimes use `OP_TRUE` and sometimes `OP_1`. I would prefer if everything could refer to that opcode in the same way as it can be confusing when switching between the two. I would prefer if we used the `OP_1` notation since the reliance on unknown segwit version behavior is an integral part of this.
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2192397657)
Concept ACK
The code, docs, and the OP sometimes use `OP_TRUE` and sometimes `OP_1`. I would prefer if everything could refer to that opcode in the same way as it can be confusing when switching between the two. I would prefer if we used the `OP_1` notation since the reliance on unknown segwit version behavior is an integral part of this.
💬 achow101 commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687048238)
In 8d956ccb86d70e2059a5e2ccd8e63b7f7c8e3842 "Add release note for P2A output feature"
nit: typos
```suggestion
and a newly recognised output template. This allows for key-less anchor
```
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687048238)
In 8d956ccb86d70e2059a5e2ccd8e63b7f7c8e3842 "Add release note for P2A output feature"
nit: typos
```suggestion
and a newly recognised output template. This allows for key-less anchor
```
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687063256)
done
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687063256)
done
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2243693286)
@achow101 swapped OP_1 for OP_TRUE everywhere :+1:
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2243693286)
@achow101 swapped OP_1 for OP_TRUE everywhere :+1:
💬 tdb3 commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243719422)
> Thanks @reardencode for the review and ACK as its obvious full-rbf should be default. There is some politics involved in it which I wanted to highlight:
Sorry for the confusion. To clarify, I had deleted my own comment. I wanted to think a bit more about it before commenting again.
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243719422)
> Thanks @reardencode for the review and ACK as its obvious full-rbf should be default. There is some politics involved in it which I wanted to highlight:
Sorry for the confusion. To clarify, I had deleted my own comment. I wanted to think a bit more about it before commenting again.
💬 1440000bytes commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243737536)
> > Thanks @reardencode for the review and ACK as its obvious full-rbf should be default. There is some politics involved in it which I wanted to highlight:
>
> Sorry for the confusion. To clarify, I had deleted my own comment. I wanted to think a bit more about it before commenting again.
1. What did you think when you commented "Concept ACK"?
2. Why did you delete it?
3. What do you think about this pull request now?
Its okay, if you refuse to answer something and not forced to do i
...
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243737536)
> > Thanks @reardencode for the review and ACK as its obvious full-rbf should be default. There is some politics involved in it which I wanted to highlight:
>
> Sorry for the confusion. To clarify, I had deleted my own comment. I wanted to think a bit more about it before commenting again.
1. What did you think when you commented "Concept ACK"?
2. Why did you delete it?
3. What do you think about this pull request now?
Its okay, if you refuse to answer something and not forced to do i
...
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687083132)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
Could you document what the return value is?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687083132)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
Could you document what the return value is?
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687085679)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
I'm a little confused about this return value. The usage of this function suggests that the return value is the wtxid of a parent transaction that has unspent dust. However, this appears to be returning the child txid.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687085679)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
I'm a little confused about this return value. The usage of this function suggests that the return value is the wtxid of a parent transaction that has unspent dust. However, this appears to be returning the child txid.
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687087037)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
`CheckEphemeralSpends` appears to only return txids, not wtxid, suggest renaming this variable:
```suggestion
const auto parent_txid = ephemeral_violation.value();
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687087037)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
`CheckEphemeralSpends` appears to only return txids, not wtxid, suggest renaming this variable:
```suggestion
const auto parent_txid = ephemeral_violation.value();
```
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687062365)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
nit: Use `contains`, here and elsewhere.
```suggestion
Assume(!map_tx_dust.contains(tx->GetHash()));
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687062365)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
nit: Use `contains`, here and elsewhere.
```suggestion
Assume(!map_tx_dust.contains(tx->GetHash()));
```
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687094839)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
It's not clear to me why there is a rule that a descendant must also spend the dust if they spent any other output from an ancestor. This rule does not seem to be mentioned anywhere in the PR description.
ISTM it would be easier and just as effective to check that all dust outputs end up being spent rather than this slightly convoluted check.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687094839)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
It's not clear to me why there is a rule that a descendant must also spend the dust if they spent any other output from an ancestor. This rule does not seem to be mentioned anywhere in the PR description.
ISTM it would be easier and just as effective to check that all dust outputs end up being spent rather than this slightly convoluted check.