💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831247308)
In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2:
nit: could make this a bit nicer with `std::ranges`:
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..569f9a95b5 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -7,7 +7,7 @@
bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
{
- return std::any_of(tx->vout.cbegin(), t
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831247308)
In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2:
nit: could make this a bit nicer with `std::ranges`:
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..569f9a95b5 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -7,7 +7,7 @@
bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
{
- return std::any_of(tx->vout.cbegin(), t
...
💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834529646)
Is there a reason for these to be `const CTransactionRef&` instead of `const CTransaction&`? The latter would avoid nullptr issues as well as improve reusability.
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..c9f1ba076d 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,12 +5,12 @@
#include <policy/ephemeral_policy.h>
#include <polic
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834529646)
Is there a reason for these to be `const CTransactionRef&` instead of `const CTransaction&`? The latter would avoid nullptr issues as well as improve reusability.
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..c9f1ba076d 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,12 +5,12 @@
#include <policy/ephemeral_policy.h>
#include <polic
...
💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830929809)
In 28733511de9073fd923d83bc43fca1353e9041aa:
nit: I would either remove these docstrings, or fix up the grammar, I spent way more time trying to parse these (and failed) than just reading the code
```
# Create two dust outputs. Both are unspent, have fees, and would have failed individual checks.
# The amount is 1 satoshi because create_self_transfer_multi disallows 0.
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830929809)
In 28733511de9073fd923d83bc43fca1353e9041aa:
nit: I would either remove these docstrings, or fix up the grammar, I spent way more time trying to parse these (and failed) than just reading the code
```
# Create two dust outputs. Both are unspent, have fees, and would have failed individual checks.
# The amount is 1 satoshi because create_self_transfer_multi disallows 0.
```
💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831408782)
I think returning a falsy value for a successful check is an antipattern. This would be nicely addressed by #25665, but until that's merged I think I'd prefer returning a `std::pair<bool, std::optional<Txid>>`, even if it's a bit more verbose:
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..10a8ab1938 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_polic
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831408782)
I think returning a falsy value for a successful check is an antipattern. This would be nicely addressed by #25665, but until that's merged I think I'd prefer returning a `std::pair<bool, std::optional<Txid>>`, even if it's a bit more verbose:
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..10a8ab1938 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_polic
...
💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832784278)
In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2
Since `MAX_DUST_OUTPUTS_PER_TX` is parameterized, it would make the tests more robust to dynamically add dust outputs too?
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 0488f8dbee..e023c99560 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -80,7 +80,7 @@ static constexpr unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT{10000};
/**
* Maximum number of
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832784278)
In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2
Since `MAX_DUST_OUTPUTS_PER_TX` is parameterized, it would make the tests more robust to dynamically add dust outputs too?
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 0488f8dbee..e023c99560 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -80,7 +80,7 @@ static constexpr unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT{10000};
/**
* Maximum number of
...
🤔 stickies-v reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2418132273)
Concept ACK. I've mostly reviewed the non-test code so far. No comments are blocking, I appreciate on a PR of this scope we can't address everything and things may need to happen in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2418132273)
Concept ACK. I've mostly reviewed the non-test code so far. No comments are blocking, I appreciate on a PR of this scope we can't address everything and things may need to happen in a follow-up.
💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834554805)
I'd prefer actually asserting this instead of just documenting it (unsure if we prefer `!IsValid()` or `IsInvalid()` here, the latter not capturing `IsError()`).
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..ac619fa385 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -931,7 +931,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// Enforces 0-fee for dust transactions,
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834554805)
I'd prefer actually asserting this instead of just documenting it (unsure if we prefer `!IsValid()` or `IsInvalid()` here, the latter not capturing `IsError()`).
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..ac619fa385 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -931,7 +931,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// Enforces 0-fee for dust transactions,
...
💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832622481)
In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2
related: the code could be simplified a bit (imo) by changing `HasDust` to `GetDust`, which would require to move it to `policy.h`
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..6066d9b3ac 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,15 +5,10 @@
#include <policy/ephemeral_policy.h>
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832622481)
In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2
related: the code could be simplified a bit (imo) by changing `HasDust` to `GetDust`, which would require to move it to `policy.h`
<details>
<summary>git diff on d5c564a24f</summary>
```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..6066d9b3ac 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,15 +5,10 @@
#include <policy/ephemeral_policy.h>
...
💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834886542)
I independently came to the same conclusion, moving my review comment here:
I feel like d147d929f5093f71c39cb7efbb0dd3888f6c8114 could just be dropped entirely? If a miner has a reason to prioritize a transaction, I don't see why they'd be okay with that not being possible just because it has a dust output? So since this is a mining RPC, I feel like practically speaking they'd just patch it out, and there'd be no real use case left for this exception, so let's just save everyone involved the
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834886542)
I independently came to the same conclusion, moving my review comment here:
I feel like d147d929f5093f71c39cb7efbb0dd3888f6c8114 could just be dropped entirely? If a miner has a reason to prioritize a transaction, I don't see why they'd be okay with that not being possible just because it has a dust output? So since this is a mining RPC, I feel like practically speaking they'd just patch it out, and there'd be no real use case left for this exception, so let's just save everyone involved the
...
💬 achow101 commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2465516616)
ACK c189eec848e3c31f438151d4d3422718a29df3a3
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2465516616)
ACK c189eec848e3c31f438151d4d3422718a29df3a3
✅ 1440000bytes closed an issue: "Add wallet RPC to get new taproot address from a silent payment address"
(https://github.com/bitcoin/bitcoin/issues/31258)
(https://github.com/bitcoin/bitcoin/issues/31258)
👍 danielabrozzoni approved a pull request: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037#pullrequestreview-2424652363)
ACK 9de9c858d5aa412e1c6086e564fbe85984a4f2d5
(https://github.com/bitcoin/bitcoin/pull/31037#pullrequestreview-2424652363)
ACK 9de9c858d5aa412e1c6086e564fbe85984a4f2d5
🚀 achow101 merged a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592)
(https://github.com/bitcoin/bitcoin/pull/30592)
👋 polespinasa's pull request is ready for review: "rpc: print P2WSH witScript in getrawtransaction"
(https://github.com/bitcoin/bitcoin/pull/31252)
(https://github.com/bitcoin/bitcoin/pull/31252)
💬 laanwj commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2465676202)
re-ACK 0de3e96e333090548a43e5e870c4cb8941d6baf1
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2465676202)
re-ACK 0de3e96e333090548a43e5e870c4cb8941d6baf1
💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1835024661)
The approach here was added to address [this comment](https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1827274569) cc @rkrux .
Which is an overkill for this simple feature honestly.
Is the simplified approach below preferable?
```python3
def run_custom_tests(self):
for method_name in self.options.test_methods:
self.log.info(f"Attempting to execute method: {method_name}")
method = getattr(self, method_name)
method()
self.log.info(f"Metho
...
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1835024661)
The approach here was added to address [this comment](https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1827274569) cc @rkrux .
Which is an overkill for this simple feature honestly.
Is the simplified approach below preferable?
```python3
def run_custom_tests(self):
for method_name in self.options.test_methods:
self.log.info(f"Attempting to execute method: {method_name}")
method = getattr(self, method_name)
method()
self.log.info(f"Metho
...
💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1835031219)
I've updated the code to use `FeeFrac`, and it simplified things – no need for tuples from the initial approach, just a vector of `FeeFrac` values which achieved same aim.
Smaller diff and no precision loss 💯
I will update all the dependent commits to follow suite.
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1835031219)
I've updated the code to use `FeeFrac`, and it simplified things – no need for tuples from the initial approach, just a vector of `FeeFrac` values which achieved same aim.
Smaller diff and no precision loss 💯
I will update all the dependent commits to follow suite.
💬 achow101 commented on pull request "Wallet: (Refactor) GetBalance to calculate used balance":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1835032532)
I don't think it makes sense to return `m_mine_used` without respecting `min_depth`. The original code passes 0 for `min_depth` because that is the default value but it needs to pass the `avoid_reuse` flag. It is not because used balance should always calculated with a min depth of 0. Ignoring it is a layer violation here.
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1835032532)
I don't think it makes sense to return `m_mine_used` without respecting `min_depth`. The original code passes 0 for `min_depth` because that is the default value but it needs to pass the `avoid_reuse` flag. It is not because used balance should always calculated with a min depth of 0. Ignoring it is a layer violation here.
💬 achow101 commented on pull request "Wallet: (Refactor) GetBalance to calculate used balance":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1835037852)
When the `avoid_reuse` flag is passed, `tx_credit_mine` and `tx_credit_mine_used` will be the same so the returned used_balance will be 0. That seems incorrect.
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1835037852)
When the `avoid_reuse` flag is passed, `tx_credit_mine` and `tx_credit_mine_used` will be the same so the returned used_balance will be 0. That seems incorrect.
💬 jb55 commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2465720609)
utACK 0de3e96e333090548a43e5e870c4cb8941d6baf1
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2465720609)
utACK 0de3e96e333090548a43e5e870c4cb8941d6baf1