🤔 glozow reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2278092199)
Looks good. New rules seem easier to reason about (up to 1 as long as 0-fee, must spend all of my parent's dust no matter what).
Can you remind me of the use cases of a keyed dust output again? It seems slightly safer to require ephemeral dust be P2A, as it would allow anybody to clean them up in case something goes wrong, but perhaps too narrow.
Additionally, could add a release note fragment.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2278092199)
Looks good. New rules seem easier to reason about (up to 1 as long as 0-fee, must spend all of my parent's dust no matter what).
Can you remind me of the use cases of a keyed dust output again? It seems slightly safer to require ephemeral dust be P2A, as it would allow anybody to clean them up in case something goes wrong, but perhaps too narrow.
Additionally, could add a release note fragment.
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1742455118)
```suggestion
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx has more than 1 dust output");
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1742455118)
```suggestion
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx has more than 1 dust output");
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744281879)
could go in test_framework/mempool_util.py?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744281879)
could go in test_framework/mempool_util.py?
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1742455496)
```suggestion
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0 fee");
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1742455496)
```suggestion
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0 fee");
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744291930)
```suggestion
self.log.info("Test that ephemeral dust is allowed for non-0 dust values")
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744291930)
```suggestion
self.log.info("Test that ephemeral dust is allowed for non-0 dust values")
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744323500)
if you make 1 a constexpr `MAX_DUST_OUTPUTS_PER_TX`, you can `static_assert` that it == 1 as an assumption in `CheckEphemeralSpends`
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744323500)
if you make 1 a constexpr `MAX_DUST_OUTPUTS_PER_TX`, you can `static_assert` that it == 1 as an assumption in `CheckEphemeralSpends`
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744296294)
I think all of these `assert_mempool_contents` can also have a `self.sync_mempools()` to check that the ephemeral dust packages propagate.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744296294)
I think all of these `assert_mempool_contents` can also have a `self.sync_mempools()` to check that the ephemeral dust packages propagate.
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744310449)
```suggestion
// Now with dust, ok because the tx has no dusty parents
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744310449)
```suggestion
// Now with dust, ok because the tx has no dusty parents
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744335056)
Could comment that `tx_input.prevout.n` doesn't need to equal `map_tx_dust.at(tx_input.prevout.hash)`; a child can spend multiple outputs of a parent.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744335056)
Could comment that `tx_input.prevout.n` doesn't need to equal `map_tx_dust.at(tx_input.prevout.hash)`; a child can spend multiple outputs of a parent.
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744312279)
```suggestion
const Txid& parent_txid = ephemeral_violation.value();
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1744312279)
```suggestion
const Txid& parent_txid = ephemeral_violation.value();
```
💬 alphachart commented on pull request "p2p: detect addnode cjdns peers in GetAddedNodeInfo()":
(https://github.com/bitcoin/bitcoin/pull/30085#issuecomment-2329956541)
Shet
(https://github.com/bitcoin/bitcoin/pull/30085#issuecomment-2329956541)
Shet
💬 furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2330067124)
> May want to update the pr description to say this bug also when happens passing a double negated value, since the not_a_boolean case is really just a special case of that
yeah, thanks. Done. PR description updated.
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2330067124)
> May want to update the pr description to say this bug also when happens passing a double negated value, since the not_a_boolean case is really just a special case of that
yeah, thanks. Done. PR description updated.
💬 theStack commented on pull request "refactor: move `SignSignature` helpers to test utils":
(https://github.com/bitcoin/bitcoin/pull/30561#issuecomment-2330113153)
Rebased on master, needed after https://github.com/bitcoin/bitcoin/pull/30784 got merged.
(https://github.com/bitcoin/bitcoin/pull/30561#issuecomment-2330113153)
Rebased on master, needed after https://github.com/bitcoin/bitcoin/pull/30784 got merged.
📝 hebasto converted_to_draft a pull request: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.
Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.
Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.
🤔 fjahr reviewed a pull request: "test: Use std::span and std::string_view for raw data"
(https://github.com/bitcoin/bitcoin/pull/30796#pullrequestreview-2281377791)
Code review ACK fa0321cc742754af4e5822d4227d4136a6f4f467
(https://github.com/bitcoin/bitcoin/pull/30796#pullrequestreview-2281377791)
Code review ACK fa0321cc742754af4e5822d4227d4136a6f4f467
💬 fjahr commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#discussion_r1744492602)
What does the `detail_` prefix mean?
(https://github.com/bitcoin/bitcoin/pull/30796#discussion_r1744492602)
What does the `detail_` prefix mean?
👋 hebasto's pull request is ready for review: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)
(https://github.com/bitcoin/bitcoin/pull/29868)
🤔 furszy reviewed a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2281468112)
Updated and rebased due to conflicts with a recently merged PR. The CI shouldn't complain now.
On a side note:
It took me longer to place my test code inside `feature_assumeutxo.py` than to fix the issue. I think it would be good to reorganize this file a bit and make the assumeUTXO parameters configurable at startup or runtime for tests. This would allow us to split tests into different files for each of the products we provide: network/p2p, local node, and wallet.
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2281468112)
Updated and rebased due to conflicts with a recently merged PR. The CI shouldn't complain now.
On a side note:
It took me longer to place my test code inside `feature_assumeutxo.py` than to fix the issue. I think it would be good to reorganize this file a bit and make the assumeUTXO parameters configurable at startup or runtime for tests. This would allow us to split tests into different files for each of the products we provide: network/p2p, local node, and wallet.
💬 benthecarman commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2330211051)
I am on 28.0rc1 and seem to be on a different chain. Block is `00000000000044a7284978db5161b2f4ca012b3fb981a9405a4ce2473f3e4a5f` for height `42545`, unsure how to get to the main chain
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2330211051)
I am on 28.0rc1 and seem to be on a different chain. Block is `00000000000044a7284978db5161b2f4ca012b3fb981a9405a4ce2473f3e4a5f` for height `42545`, unsure how to get to the main chain
📝 marcofleon opened a pull request: "doc: fix assumeutxo design doc link"
(https://github.com/bitcoin/bitcoin/pull/30819)
A correction to a link as I was exploring Assumeutxo stuff.
(https://github.com/bitcoin/bitcoin/pull/30819)
A correction to a link as I was exploring Assumeutxo stuff.