💬 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
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470140068)
> I'm not sure how to proceed on this, might need input from other reviewers. `"jsonrpc":"1.0"` is not a standard anywhere, not even in Bitcoin... but it has been in the RPC help text for a looooong time 😭
This is a good find, and I haven't looked closely, but it seems like there is not any conflict here. We can continue to accept `"jsonrpc":"1.0"` for compatibility, continue to require `"jsonrpc":"2.0" for JSON-RPC 2.0 behavior to comply with the 2.0 spec, and reject any other values so if
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470140068)
> I'm not sure how to proceed on this, might need input from other reviewers. `"jsonrpc":"1.0"` is not a standard anywhere, not even in Bitcoin... but it has been in the RPC help text for a looooong time 😭
This is a good find, and I haven't looked closely, but it seems like there is not any conflict here. We can continue to accept `"jsonrpc":"1.0"` for compatibility, continue to require `"jsonrpc":"2.0" for JSON-RPC 2.0 behavior to comply with the 2.0 spec, and reject any other values so if
...
💬 josibake commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1915469470)
ACK https://github.com/bitcoin/bitcoin/pull/29347/commits/292e716cde3325bef83e78f7804d2d0bddf03509 :rocket:
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1915469470)
ACK https://github.com/bitcoin/bitcoin/pull/29347/commits/292e716cde3325bef83e78f7804d2d0bddf03509 :rocket:
👍 kristapsk approved a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1849674537)
cr utACK 292e716cde3325bef83e78f7804d2d0bddf03509
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1849674537)
cr utACK 292e716cde3325bef83e78f7804d2d0bddf03509
💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1470160697)
would still like a good explanation for this
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1470160697)
would still like a good explanation for this
💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1470161011)
still waiting on some feedback on this point before moving forward with the PR
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1470161011)
still waiting on some feedback on this point before moving forward with the PR
💬 instagibbs commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1915501310)
ACK https://github.com/bitcoin/bitcoin/pull/29347/commits/292e716cde3325bef83e78f7804d2d0bddf03509
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1915501310)
ACK https://github.com/bitcoin/bitcoin/pull/29347/commits/292e716cde3325bef83e78f7804d2d0bddf03509
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470185755)
ok, carved out `"jsonrpc":"1.0"` and tested with push to ec5e7cea2bddfd55f07f9b0654f11d60f5ab0a48
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470185755)
ok, carved out `"jsonrpc":"1.0"` and tested with push to ec5e7cea2bddfd55f07f9b0654f11d60f5ab0a48
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1915548465)
I think the discussion on this PR is so spirited because it speaks to a more fundamental question of how to make changes to the bitcoin network. Please let me know if there is a more appropriate forum for this discussion, I'm happy to take this elsewhere.
> @chrisguida Let me expand on my NACK: given how easy it is for miners to adopt profit-maximizing transaction policies, and how easy it is for people to relay profit-maximizing transactions to them, we should not try to filter out profitabl
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1915548465)
I think the discussion on this PR is so spirited because it speaks to a more fundamental question of how to make changes to the bitcoin network. Please let me know if there is a more appropriate forum for this discussion, I'm happy to take this elsewhere.
> @chrisguida Let me expand on my NACK: given how easy it is for miners to adopt profit-maximizing transaction policies, and how easy it is for people to relay profit-maximizing transactions to them, we should not try to filter out profitabl
...
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915579486)
Thank you for your comments @josibake @glozow @achow101 , will address review comments shortly.
> if you typo'd a feerate orders of magnitude higher, your maxtxfee should get hit.
I don't think this is true; it depends on what your `maxtxfee` is. Going by the default value of 0.10 BTC.
This is not likely to occur.
I did a manual test on master.
When I set `maxtxfee` to 0.10 BTC on master, if you mistakenly type 10,000 as the fee rate, the max fee exceed error will not be hit.
...
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915579486)
Thank you for your comments @josibake @glozow @achow101 , will address review comments shortly.
> if you typo'd a feerate orders of magnitude higher, your maxtxfee should get hit.
I don't think this is true; it depends on what your `maxtxfee` is. Going by the default value of 0.10 BTC.
This is not likely to occur.
I did a manual test on master.
When I set `maxtxfee` to 0.10 BTC on master, if you mistakenly type 10,000 as the fee rate, the max fee exceed error will not be hit.
...
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915582287)
> just seems really gross to have to now consider both of these in coin selection, especially if a user sets wonky values.
I dont really understand how this can affect coinselection, can you expand please? Thanks
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915582287)
> just seems really gross to have to now consider both of these in coin selection, especially if a user sets wonky values.
I dont really understand how this can affect coinselection, can you expand please? Thanks