💬 mzumsande commented on pull request "redeclare nChainTx to use uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1915311442)
> It should be possible to fake a large `nChainTx` in a test using assume utxo
It's annoying that each test doing something like that would need to add an entry to the regtest chainparams, maybe my suggestion [here](https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1907059722) to allow that per RPC could make sense (not necessarily in this PR).
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1915311442)
> It should be possible to fake a large `nChainTx` in a test using assume utxo
It's annoying that each test doing something like that would need to add an entry to the regtest chainparams, maybe my suggestion [here](https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1907059722) to allow that per RPC could make sense (not necessarily in this PR).
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470021935)
> do we need a separate `_send_lock` for every P2PConnection? benchmarks look slightly faster with a global `_send_lock` (like `p2p_lock`).
Umm, that sounds a bit strange to me. My intuition would be that having a global lock should be slower, especially if multiple connections are run at the same time, given they would be racing to acquire that lock every time a message needs to be sent (?)
w.r.t wether or not a deadlock is possible, I think @mzumsande may be able to tell you better given
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470021935)
> do we need a separate `_send_lock` for every P2PConnection? benchmarks look slightly faster with a global `_send_lock` (like `p2p_lock`).
Umm, that sounds a bit strange to me. My intuition would be that having a global lock should be slower, especially if multiple connections are run at the same time, given they would be racing to acquire that lock every time a message needs to be sent (?)
w.r.t wether or not a deadlock is possible, I think @mzumsande may be able to tell you better given
...
💬 1440000bytes commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915317702)
> We are facing a problem in BTCPay: A large number of users have only 20GB of space to store the UTXO Set. Now that the UTXO Set is reaching 10GB and increasing, those users might have their node crashing sooner or later. So we are very interested into solutions to drop down the UTXO set size.
This pull request won't make any difference. Top 5 output types in UTXO set are P2PKH, P2WPKH, P2TR, P2SH and P2WSH.
https://txstats.com/d/000000054/utxo-set-repartition-by-output-type?orgId=1
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915317702)
> We are facing a problem in BTCPay: A large number of users have only 20GB of space to store the UTXO Set. Now that the UTXO Set is reaching 10GB and increasing, those users might have their node crashing sooner or later. So we are very interested into solutions to drop down the UTXO set size.
This pull request won't make any difference. Top 5 output types in UTXO set are P2PKH, P2WPKH, P2TR, P2SH and P2WSH.
https://txstats.com/d/000000054/utxo-set-repartition-by-output-type?orgId=1
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915336928)
> I think it makes sense to have both. I could have a `maxtxfee` of the upper bound of what I'm willing to pay ever, and also set `maxfeerate` to something reasonable just to make sure I don't typo a feerate and accidentally pay a feerate orders of magnitude higher than intended. At `maxfeerate`, a small transaction can still pay a smaller fee than `maxtxfee`, but a large transaction could exceed `maxtxfee`.
But in this example, if you typo'd a feerate orders of magnitude higher, your `maxtxf
...
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915336928)
> I think it makes sense to have both. I could have a `maxtxfee` of the upper bound of what I'm willing to pay ever, and also set `maxfeerate` to something reasonable just to make sure I don't typo a feerate and accidentally pay a feerate orders of magnitude higher than intended. At `maxfeerate`, a small transaction can still pay a smaller fee than `maxtxfee`, but a large transaction could exceed `maxtxfee`.
But in this example, if you typo'd a feerate orders of magnitude higher, your `maxtxf
...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470044262)
Hm this is a bit of a mess, you're right. The behavior in this PR makes sense in response to @ryanofsky comment: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446841611
The help text has also been modified in `src/rpc/util.cpp` (98031401c744ea3c2b62927fabf27c22b639c3cf) to always recommend `"jsonrpc":"2.0"`
I think you are illustrating an important issue though -- anyone who has used `bitcoin-cli help` (for ANY RPC command!) would have seen instructions to include `"jsonrpc":
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470044262)
Hm this is a bit of a mess, you're right. The behavior in this PR makes sense in response to @ryanofsky comment: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446841611
The help text has also been modified in `src/rpc/util.cpp` (98031401c744ea3c2b62927fabf27c22b639c3cf) to always recommend `"jsonrpc":"2.0"`
I think you are illustrating an important issue though -- anyone who has used `bitcoin-cli help` (for ANY RPC command!) would have seen instructions to include `"jsonrpc":
...
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1470045451)
I believe you're correct that this code is not reachable in a functional test, but the reason is that when PV3C(TxB) is invoked, we'll notice that it is a v3 child and that it has an in-package descendant, and fail further down (https://github.com/bitcoin/bitcoin/pull/28948/commits/965a23f33404b5e3cac41b7c4e17298e96ff25bf#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R118-R120).
There is a test for this topology in the functional tests, see `mempool_accept_v3.py` here:
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1470045451)
I believe you're correct that this code is not reachable in a functional test, but the reason is that when PV3C(TxB) is invoked, we'll notice that it is a v3 child and that it has an in-package descendant, and fail further down (https://github.com/bitcoin/bitcoin/pull/28948/commits/965a23f33404b5e3cac41b7c4e17298e96ff25bf#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R118-R120).
There is a test for this topology in the functional tests, see `mempool_accept_v3.py` here:
...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470059890)
This is a good catch but unfortunately I think we need to stick with this behavior for backwards compatibility. If we get a request that does not have the exact key-value pair `"jsonrpc":"2.0"` we have to resort to legacy behavior. In that case, the user has submitted an empty batch and what they get back is an empty vector.
The closest 2.0 example I can think of is this:
```
--> [{"jsonrpc":"2.0", "id":""}]
<-- [{'jsonrpc': '2.0', 'error': {'code': -32600, 'message': 'Missing method'},
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470059890)
This is a good catch but unfortunately I think we need to stick with this behavior for backwards compatibility. If we get a request that does not have the exact key-value pair `"jsonrpc":"2.0"` we have to resort to legacy behavior. In that case, the user has submitted an empty batch and what they get back is an empty vector.
The closest 2.0 example I can think of is this:
```
--> [{"jsonrpc":"2.0", "id":""}]
<-- [{'jsonrpc': '2.0', 'error': {'code': -32600, 'message': 'Missing method'},
...
💬 russeree commented on issue "Policy: disallow P2PK transactions from relaying by default":
(https://github.com/bitcoin/bitcoin/issues/29285#issuecomment-1915386230)
P2PK would be the first place people using stamps would go to if relay policy for P2MS became to restrictive.
(https://github.com/bitcoin/bitcoin/issues/29285#issuecomment-1915386230)
P2PK would be the first place people using stamps would go to if relay policy for P2MS became to restrictive.
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470077827)
Thanks!!!
Fixed in force push to 771d1e1d206efe687b8661ab966cc1a62cc7ba39
```diff
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 54354a8625..f678668fd3 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -247,6 +247,11 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
}
if (!jreq.IsNotification()) reply.push_back(std::move(response));
}
+ if (reply.empty()) {
+ // All-noti
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470077827)
Thanks!!!
Fixed in force push to 771d1e1d206efe687b8661ab966cc1a62cc7ba39
```diff
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 54354a8625..f678668fd3 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -247,6 +247,11 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
}
if (!jreq.IsNotification()) reply.push_back(std::move(response));
}
+ if (reply.empty()) {
+ // All-noti
...
📝 sipa opened a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347)
This enables BIP324's v2 transport by default (see #27634):
* Inbound connections will auto-sense whether v1 or v2 is in use.
* Automatic outbound connections will use v2 if NODE_P2P_V2 was set in addr gossip, but retry with v1 if met with immediate failure.
* Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.
(https://github.com/bitcoin/bitcoin/pull/29347)
This enables BIP324's v2 transport by default (see #27634):
* Inbound connections will auto-sense whether v1 or v2 is in use.
* Automatic outbound connections will use v2 if NODE_P2P_V2 was set in addr gossip, but retry with v1 if met with immediate failure.
* Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1470102231)
As @instagibbs pointed out elsewhere, we don't need to check these ancestor_counts in `PackageV3Checks` after all, so we can get rid of this struct simply pass in the `Package` to PV3C.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1470102231)
As @instagibbs pointed out elsewhere, we don't need to check these ancestor_counts in `PackageV3Checks` after all, so we can get rid of this struct simply pass in the `Package` to PV3C.
👍 ryanofsky approved a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1849338675)
Code review ACK 493cfc728aa7835527b1eab179b8cb36edd60946. I left more more comments about comments (feel free to ignore), but otherwise this looks good.
re: https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1911119751
> > In general, we don't have very robust error handling for when the database fails to read or write something. I think this should actually be more like an assert, but one that doesn't kill the whole software, maybe just unloads the wallet?
>
> It wouldn't be id
...
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1849338675)
Code review ACK 493cfc728aa7835527b1eab179b8cb36edd60946. I left more more comments about comments (feel free to ignore), but otherwise this looks good.
re: https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1911119751
> > In general, we don't have very robust error handling for when the database fails to read or write something. I think this should actually be more like an assert, but one that doesn't kill the whole software, maybe just unloads the wallet?
>
> It wouldn't be id
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1469966356)
In commit "sqlite: add ability to interrupt statements" (53c4f1334ee300e2228974b4abca51c38087c51c)
Would suggest a rename and comment.
This class is SQLite specific, but unlike other other classes in this file it does not have SQLite in the name. Would suggest calling it `SQliteExecInterface` since it is an abstract class with all virtual methods.
Also, it's not initially clear why this class and the class below exist. Would suggest adding a doxygen comment like //* Abstract class respo
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1469966356)
In commit "sqlite: add ability to interrupt statements" (53c4f1334ee300e2228974b4abca51c38087c51c)
Would suggest a rename and comment.
This class is SQLite specific, but unlike other other classes in this file it does not have SQLite in the name. Would suggest calling it `SQliteExecInterface` since it is an abstract class with all virtual methods.
Also, it's not initially clear why this class and the class below exist. Would suggest adding a doxygen comment like //* Abstract class respo
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1470026684)
In commit "sqlite: guard against dangling to-be-reverted db transactions" (0bf3a7356db224d28a86194b004ca30549197f27)
I think this comment might be more harmful than helpful in its current form, because it isn't communicating the most important thing, which is that this code path is never expected to be hit under normal conditions, and if it is hit, it means something is seriously wrong and there has probably been data loss, if not data corruption. By saying this failure can happen when the co
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1470026684)
In commit "sqlite: guard against dangling to-be-reverted db transactions" (0bf3a7356db224d28a86194b004ca30549197f27)
I think this comment might be more harmful than helpful in its current form, because it isn't communicating the most important thing, which is that this code path is never expected to be hit under normal conditions, and if it is hit, it means something is seriously wrong and there has probably been data loss, if not data corruption. By saying this failure can happen when the co
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1470035536)
re: https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466968974
> I tend to call the `WalletBatch` handler because 'batch' refers to a class that groups operations and dumps the result all at once atomically at the end of the process.
:grinning: yeah very used to seeing bland names that don't mean anything because someone rejected an an actually descriptive name for a pedantic reason. In this case, I think a batch is just a collection of actions done together. Metaphors are ok onc
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1470035536)
re: https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466968974
> I tend to call the `WalletBatch` handler because 'batch' refers to a class that groups operations and dumps the result all at once atomically at the end of the process.
:grinning: yeah very used to seeing bland names that don't mean anything because someone rejected an an actually descriptive name for a pedantic reason. In this case, I think a batch is just a collection of actions done together. Metaphors are ok onc
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1470113559)
I cleaned up the Optimality fuzz test a bit:
- I have removed multiple CoinSelectionParams from being fuzzed that are not relevant to testing CoinGrinder’s optimality (SFFO, LTFRE, all change cost related quantities except `min_change_target`)
- I now fuzz `min_change_target`
- I just create up to 16 UTXOs that are each a separate OutputGroup
- I now first run the brute force solution, then set a fuzzed max_weight greater than the best_weight and check that CoinGrinder finds the optimal so
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1470113559)
I cleaned up the Optimality fuzz test a bit:
- I have removed multiple CoinSelectionParams from being fuzzed that are not relevant to testing CoinGrinder’s optimality (SFFO, LTFRE, all change cost related quantities except `min_change_target`)
- I now fuzz `min_change_target`
- I just create up to 16 UTXOs that are each a separate OutputGroup
- I now first run the brute force solution, then set a fuzzed max_weight greater than the best_weight and check that CoinGrinder finds the optimal so
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1470115876)
I have decided to defer this for the moment, since I do appreciate the clarity of distinguishing between the `best_selection_weight` and `max_weight`, and I anticipate that I will have to revisit this in the context of https://github.com/bitcoin/bitcoin/pull/29264 soon anyway.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1470115876)
I have decided to defer this for the moment, since I do appreciate the clarity of distinguishing between the `best_selection_weight` and `max_weight`, and I anticipate that I will have to revisit this in the context of https://github.com/bitcoin/bitcoin/pull/29264 soon anyway.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1915438014)
All review feedback is addressed, ready for review.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1915438014)
All review feedback is addressed, ready for review.
💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1470126512)
re: https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1467815704
> But if there's only one name here, it's unclear what name the bool type is for? Or we're assuming it can only be used as a positional param at that point?
Yes, that's a good point. The change I suggested to `wallet/rpc/spend.cpp` makes the code less clear, even if it is removes some code that is technically unnecessary. So your version is better.
In case anybody is following this though, the github comment this i
...
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1470126512)
re: https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1467815704
> But if there's only one name here, it's unclear what name the bool type is for? Or we're assuming it can only be used as a positional param at that point?
Yes, that's a good point. The change I suggested to `wallet/rpc/spend.cpp` makes the code less clear, even if it is removes some code that is technically unnecessary. So your version is better.
In case anybody is following this though, the github comment this i
...
💬 sdaftuar commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1915460898)
Thanks @t-bast and @TheBlueMatt for chiming in here.
> It does require different package RBF rules than BIP 125 though:
>
> * we mustn't apply BIP 125 rules 2 and 3
Relaxing rule 3 is the main issue, in my view, as it stems from concerns around both incentive compatibility (which I explore in my cluster mempool post [here](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#rbf-can-now-be-made-incentive-compatible-for-miners-11)) and anti-DoS protections against
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1915460898)
Thanks @t-bast and @TheBlueMatt for chiming in here.
> It does require different package RBF rules than BIP 125 though:
>
> * we mustn't apply BIP 125 rules 2 and 3
Relaxing rule 3 is the main issue, in my view, as it stems from concerns around both incentive compatibility (which I explore in my cluster mempool post [here](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#rbf-can-now-be-made-incentive-compatible-for-miners-11)) and anti-DoS protections against
...