💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469908727)
this doesn't have coverage in functional or unit tests, though I'm not sure this can be hit?

TxA(in mempool) -> TxB -> TxC

this is the minimal topology, right? However, this condition will trigger failure at https://github.com/bitcoin/bitcoin/pull/28948/files#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R53

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469979561)
maybe use `TX_MAX_STANDARD_VERSION` instead

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469896676)
1005 is pretty magical, though I realize we have no way of actually computing vsize of a child not in mempool. sad!

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469866501)
```suggestion
std::optional<std::string> SingleV3Rules(const CTransactionRef& ptx,
```
can leave a comment saying that it can *also* be called on transactions in packages, as an optimization.

💬 achow101 commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915276592)
> Or is there a use case for caring about the total fees you pay AND caring about the sats per vbyte you pay that I'm not understanding?

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

💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1915302735)
> Yes, but we just derive the public key from the private key. There's no need to store it and check it. If it got corrupted, you just make a new one.

If bitcoin core is not generating this key, then it seems reasonable to have the pubkey along with it so that it can be verified. If bitcoin core *is* generating this key, I don't see why we are writing it to disk unencrypted. Seems like a bad idea. Also it seems like these keys can be ephemeral if we can just make a new one, so writing it to d
...

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

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

💬 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

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

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

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

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

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

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

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

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

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

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

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