💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1834783665)
Indeed, removed.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1834783665)
Indeed, removed.
💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2465442292)
> Yeah, don't worry about the ctest bug. It is unrelated and should be fixed somewhere else. I was mostly wondering why a force push that does not change behavior ([#31000 (comment)](https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680)) suddenly makes the CI trip. Why would CI pass before and not after, when `-testdatadir` isn't set. Maybe someone can run the two commits locally on Windows?
Ok, the force-push actually contained a behavior change. It changed the default test
...
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2465442292)
> Yeah, don't worry about the ctest bug. It is unrelated and should be fixed somewhere else. I was mostly wondering why a force push that does not change behavior ([#31000 (comment)](https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680)) suddenly makes the CI trip. Why would CI pass before and not after, when `-testdatadir` isn't set. Maybe someone can run the two commits locally on Windows?
Ok, the force-push actually contained a behavior change. It changed the default test
...
📝 furszy opened a pull request: "ci: make ctest stop on failure"
(https://github.com/bitcoin/bitcoin/pull/31257)
Make `ctest` stops when the first failure happens.
Wasting less resources and notifying the developer faster when a failure occurs.
(https://github.com/bitcoin/bitcoin/pull/31257)
Make `ctest` stops when the first failure happens.
Wasting less resources and notifying the developer faster when a failure occurs.
⚠️ 1440000bytes opened an issue: "Add wallet RPC to get new taproot address from a silent payment address"
(https://github.com/bitcoin/bitcoin/issues/31258)
### Please describe the feature you'd like to see added.
RPC that allows generating addresses from a given silent payment address.
### Is your feature related to a problem, if so please describe it.
This RPC could help application developers that use bitcoin core RPC in different projects to generate new addresses from a given silent payment address.
### Describe the solution you'd like
[`getnewaddress`](https://bitcoincore.org/en/doc/28.0.0/rpc/wallet/getnewaddress/) could have a new argum
...
(https://github.com/bitcoin/bitcoin/issues/31258)
### Please describe the feature you'd like to see added.
RPC that allows generating addresses from a given silent payment address.
### Is your feature related to a problem, if so please describe it.
This RPC could help application developers that use bitcoin core RPC in different projects to generate new addresses from a given silent payment address.
### Describe the solution you'd like
[`getnewaddress`](https://bitcoincore.org/en/doc/28.0.0/rpc/wallet/getnewaddress/) could have a new argum
...
💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2465480709)
Updated. Ready to go.
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2465480709)
Updated. Ready to go.
💬 achow101 commented on issue "Add wallet RPC to get new taproot address from a silent payment address":
(https://github.com/bitcoin/bitcoin/issues/31258#issuecomment-2465484175)
Given that computing the actual output script in a transaction paying to a silent payments address requires knowing the exact inputs and having the private keys for those inputs, I don't think this will really be possible in an ergonomic way, and any way to implement this is very footgun-y. The taproot address is entirely dependent on the inputs used so the receiver does not necessarily know that they have been paid if you just send to any taproot address computed from a silent payments address.
...
(https://github.com/bitcoin/bitcoin/issues/31258#issuecomment-2465484175)
Given that computing the actual output script in a transaction paying to a silent payments address requires knowing the exact inputs and having the private keys for those inputs, I don't think this will really be possible in an ergonomic way, and any way to implement this is very footgun-y. The taproot address is entirely dependent on the inputs used so the receiver does not necessarily know that they have been paid if you just send to any taproot address computed from a silent payments address.
...
🤔 BrandonOdiwuor reviewed a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2424605718)
Re-ACK 83fab3212c91d91fc5502f940c901a07772ff747
(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
(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)
(https://github.com/bitcoin/bitcoin/issues/29552)
🚀 achow101 merged a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686)
(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
...
(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
...
(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
...