💬 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

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

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

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

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

💬 jb55 commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2465720609)
utACK 0de3e96e333090548a43e5e870c4cb8941d6baf1

💬 mzumsande commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2465734085)
> (yes, I think it is a subset of bind=, somebody correct me if I am wrong)

As far as I understand it, one difference is that `-bind` disables self-discovery of addresses because `connOptions.bind_on_any` will not be set - whereas `-port` will work together with bind-on-any. So if you want to use `Discover()` together with a non-default port, `-port` is the only option.

👍 darosior approved a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2424940909)
ACK 6a00ea17c136e67a66f3558dd2d02a07860b0afe

💬 darosior commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2465795279)
> What about having P2P-seeds, an alternative to DNS-seeds

I guess the biggest drawback with this is how bootstrapping nodes will actually connect to this "P2P-seeds" instead of leveraging the DNS cache of their ISP.

💬 darosior commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2465821538)
Besides the load, which can be dealt with by having a large number of seeds, i wonder how much getting rid of the DNS caching impacts "privacy", loosely defined. Of course from the perspective of a node, if you want to hide your IP you use an anonymizing network period. But it's also the case that being a P2P seed gives you a privileged observation position on all the nodes joining the network, which is less true for DNS seeds since most of the time the bootstrapping nodes would not actually con
...

🤔 hodlinator reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2420454069)
Concept ACK 131bed19bdfc922328cad9781fa9122b6810a8fd

Thanks for working on this! Seems like an obvious win for L2s.

Haven't yet absorbed the full context sufficiently to fully A-C-K it myself. Hopefully my comments here and above provide value even though most are surface level. My personal priority with this review is to get it in better shape for others so they can A-C-K more easily if they agree.

💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832718055)
```suggestion
// so the last transaction to be generated (in a >1 package) must spend all package-made outputs
```