🤔 BrandonOdiwuor reviewed a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2424605718)
Re-ACK 83fab3212c91d91fc5502f940c901a07772ff747

💬 achow101 commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#issuecomment-2465487012)
ACK 47f50c7af5572520fd986b313a63a44a76d3c859

✅ achow101 closed an issue: "man pages have a useless descriptions"
(https://github.com/bitcoin/bitcoin/issues/29552)

🚀 achow101 merged a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686)

💬 stickies-v commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831195216)
nit: includes are [not quite up to date](https://api.cirrus-ci.com/v1/task/6598898733547520/logs/ci.log) (in both files)

<details>
<summary>git diff on d5c564a24f to make iwyu happy</summary>

```diff
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..3a9a3fa530 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,6 +5,22 @@
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>

+#includ
...

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

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

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

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

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

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

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

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

...

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

💬 achow101 commented on pull request "Remove mempoolfullrbf":
(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)

👍 danielabrozzoni approved a pull request: "test: enhance p2p_orphan_handling"
(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)

👋 polespinasa's pull request is ready for review: "rpc: print P2WSH witScript in getrawtransaction"
(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