🤔 ryanofsky reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1883484751)
Code review c891a65067f57ba87e443d0cdc4f7e6b434c436f.
Thanks for the updates! A lot of nice changes and test coverage here, and I think the PR looks close to ready to ACK, though I did leave a number of suggestions below. Most of these should be very simple or simplify code, so should not create too much work.
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1883484751)
Code review c891a65067f57ba87e443d0cdc4f7e6b434c436f.
Thanks for the updates! A lot of nice changes and test coverage here, and I think the PR looks close to ready to ACK, though I did leave a number of suggestions below. Most of these should be very simple or simplify code, so should not create too much work.
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491449136)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
I was surprised to see this change, since I thought intent of the preceding commit was to preserve current behavior and consider spending transactions active even they conflicted with mempool transactions. Both approaches seem ok, but if this behavior is intended, it seems like it would be simpler to write this condition as:
```c++
if (!wtx.IsAbandoned() && !wtx.isConflicted()) {
return true; // sp
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491449136)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
I was surprised to see this change, since I thought intent of the preceding commit was to preserve current behavior and consider spending transactions active even they conflicted with mempool transactions. Both approaches seem ok, but if this behavior is intended, it seems like it would be simpler to write this condition as:
```c++
if (!wtx.IsAbandoned() && !wtx.isConflicted()) {
return true; // sp
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491442105)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Would suggest expanding comment to something like: "Set of mempool transactions that conflict directly with the tx or an ancestor transaction. This set will be empty if the transaction is confirmed, but can be nonempty in other states."
I was initially surprised to see that this set was populated not just for inactive transactions, but also for block-conflicted transactions, so think it is worth mentioni
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491442105)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Would suggest expanding comment to something like: "Set of mempool transactions that conflict directly with the tx or an ancestor transaction. This set will be empty if the transaction is confirmed, but can be nonempty in other states."
I was initially surprised to see that this set was populated not just for inactive transactions, but also for block-conflicted transactions, so think it is worth mentioni
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491412469)
In commit "scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted" (df0130d73341bb2784b23cd2e8307f30da09ca37)
Not very important, but I think it would be good if this scripted-diff commit renamed `CWalletTx::isConflicted` to `isBlockConflicted` up front, so `isConflicted` could be added as a new method later without affecting existing callers. Otherwise it takes extra effort for reviewers to look up all the existing callers of isConflicted recursively and making sure their behavior
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491412469)
In commit "scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted" (df0130d73341bb2784b23cd2e8307f30da09ca37)
Not very important, but I think it would be good if this scripted-diff commit renamed `CWalletTx::isConflicted` to `isBlockConflicted` up front, so `isConflicted` could be added as a new method later without affecting existing callers. Otherwise it takes extra effort for reviewers to look up all the existing callers of isConflicted recursively and making sure their behavior
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491576720)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It seems wasteful and potentially slow to use RecursiveUpdateTxState in its current form because it will be calling WalletBatch::WriteTx for these transactions and updating the database even though the transaction data will not be changing.
Would suggest fixing this by adding a `CWallet::RecursiveUpdateTxState` overload that takes a `WalletBatch*` parameter and does not call `WriteTx` when it is null. E.
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491576720)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It seems wasteful and potentially slow to use RecursiveUpdateTxState in its current form because it will be calling WalletBatch::WriteTx for these transactions and updating the database even though the transaction data will not be changing.
Would suggest fixing this by adding a `CWallet::RecursiveUpdateTxState` overload that takes a `WalletBatch*` parameter and does not call `WriteTx` when it is null. E.
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491434583)
In commit "wallet: use CWalletTx member functions to determine tx state" (523a48b7180409970cec286f0aef6afbbacd2ffa)
This code comment is just repeating the boolean expression below without explaining it. Would suggest a comment explaining what it is trying to do like "An output is considered spent by a spending transaction unless the spending transaction is conflicted or abandoned. The check below is written conservatively to check for the inverse condition and only consider spending transact
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491434583)
In commit "wallet: use CWalletTx member functions to determine tx state" (523a48b7180409970cec286f0aef6afbbacd2ffa)
This code comment is just repeating the boolean expression below without explaining it. Would suggest a comment explaining what it is trying to do like "An output is considered spent by a spending transaction unless the spending transaction is conflicted or abandoned. The check below is written conservatively to check for the inverse condition and only consider spending transact
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491468929)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Note: effects of changing this definition seem to be:
- `CachedTxIsTrusted` is changed, so if a mempool conflict with a transaction is detected, the transaction will no longer be trusted, and its outputs will show up in the unconfirmed balance and no longer be spent from.
- `isUnconfirmed` is changed, so `ResubmitWalletTransactions` is changed, so if a mempool conflict with a transaction is detected, th
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491468929)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Note: effects of changing this definition seem to be:
- `CachedTxIsTrusted` is changed, so if a mempool conflict with a transaction is detected, the transaction will no longer be trusted, and its outputs will show up in the unconfirmed balance and no longer be spent from.
- `isUnconfirmed` is changed, so `ResubmitWalletTransactions` is changed, so if a mempool conflict with a transaction is detected, th
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491562385)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Seems like a minor bug here. If this returns `TxUpdate::UNCHANGED` it means RecursiveUpdateTxState will stop recursing, so the parent transaction will have its `mempool_conflicts` list updated, but its descendants will not.
In practice, this should be a minor bug because the only time `TxUpdate::UNCHANGED` is actually returned here is when the transaction is in `BlockConflicted` state, and it shouldn't b
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491562385)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Seems like a minor bug here. If this returns `TxUpdate::UNCHANGED` it means RecursiveUpdateTxState will stop recursing, so the parent transaction will have its `mempool_conflicts` list updated, but its descendants will not.
In practice, this should be a minor bug because the only time `TxUpdate::UNCHANGED` is actually returned here is when the transaction is in `BlockConflicted` state, and it shouldn't b
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491512496)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It doesn't seem actually seem useful to add this `TxStateInactive::mempool_conflicted` member, so would suggest dropping it and changing definition of `isMempoolConflicted()` to just return `!mempool_conflicts.empty()`.
When I made earlier suggestion to add this member in https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1936304636 I didn't dig into the relationship between the mempool conflicte
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491512496)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It doesn't seem actually seem useful to add this `TxStateInactive::mempool_conflicted` member, so would suggest dropping it and changing definition of `isMempoolConflicted()` to just return `!mempool_conflicts.empty()`.
When I made earlier suggestion to add this member in https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1936304636 I didn't dig into the relationship between the mempool conflicte
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491593170)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
This has the same problems as the new transactionAddedToMempool code above. Would suggest avoiding the need for state transitions, and always updating the mempool_conflicts set recursively for BlockConflicted as well as Inactive transactions:
```c++
if (!wtx.mempool_conflicts.erase(conflict_txid)) return TxUpdate::UNCHANGED;
return TxUpdate::CHANGED;
```
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491593170)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
This has the same problems as the new transactionAddedToMempool code above. Would suggest avoiding the need for state transitions, and always updating the mempool_conflicts set recursively for BlockConflicted as well as Inactive transactions:
```c++
if (!wtx.mempool_conflicts.erase(conflict_txid)) return TxUpdate::UNCHANGED;
return TxUpdate::CHANGED;
```
💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1947331502)
Ah, good point. So it looks like right now this is a refactor, and shouldn't change the binary on any platform? If you agree, you can add the `refactor: ` prefix to the pull request title.
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1947331502)
Ah, good point. So it looks like right now this is a refactor, and shouldn't change the binary on any platform? If you agree, you can add the `refactor: ` prefix to the pull request title.
💬 mzumsande commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491664859)
ok, makes sense!
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491664859)
ok, makes sense!
👍 hernanmarino approved a pull request: "doc: Update translation process guide"
(https://github.com/bitcoin/bitcoin/pull/29414#pullrequestreview-1883922565)
ACK 3b0ec06d6228d965e9cf9121c5dd300da2a930ea
It might be interesting to add to this guide the details expressed in this comment https://github.com/bitcoin-core/gui/pull/793#issuecomment-1943421925
(https://github.com/bitcoin/bitcoin/pull/29414#pullrequestreview-1883922565)
ACK 3b0ec06d6228d965e9cf9121c5dd300da2a930ea
It might be interesting to add to this guide the details expressed in this comment https://github.com/bitcoin-core/gui/pull/793#issuecomment-1943421925
💬 DoctorBuzz1 commented on issue "Move from Static Dust Limit [330 / 546 sats] to Variable Dust Limit [= to TxFee]":
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947534729)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Soo.... if we take my 2nd proposal of
`if (3Tx < TxFee) {isDust = True;}`
And adjust it just a tad to:
`if (4Tx < TxFee) {isDust = True;}`
Then that Tx would be able to be relayed & mined. Any more examples?
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947534729)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Soo.... if we take my 2nd proposal of
`if (3Tx < TxFee) {isDust = True;}`
And adjust it just a tad to:
`if (4Tx < TxFee) {isDust = True;}`
Then that Tx would be able to be relayed & mined. Any more examples?
💬 kevkevinpal commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491842216)
doesn't the `+1` make the `garbage_len` greater than `4095` which is what is being covered in `TestType.EXCESS_GARBAGE`
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491842216)
doesn't the `+1` make the `garbage_len` greater than `4095` which is what is being covered in `TestType.EXCESS_GARBAGE`
💬 kevkevinpal commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491843089)
never mind randrange doesn't include the value passed
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491843089)
never mind randrange doesn't include the value passed
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491901555)
yes!
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491901555)
yes!
💬 DoctorBuzz1 commented on issue "Move from Static Dust Limit [330 / 546 sats] to Variable Dust Limit [= to TxFee]":
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947681819)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Are you able to explain what's happening in this Tx? Why is it overpaying 3x what's necessary to get into the block when 25 sats/vByte definiely would've made it into the next block?
If the TxFee is 25,582 sats.... how is that distributed amo
...
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947681819)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Are you able to explain what's happening in this Tx? Why is it overpaying 3x what's necessary to get into the block when 25 sats/vByte definiely would've made it into the next block?
If the TxFee is 25,582 sats.... how is that distributed amo
...
💬 glozow commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1947800641)
Package Relay #27463
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1947800641)
Package Relay #27463
💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1947928214)
Ok, this (The signed, previous to last commit) is ready for review.
I'll drop the last commit (merge commit) once and if it is merged.
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1947928214)
Ok, this (The signed, previous to last commit) is ready for review.
I'll drop the last commit (merge commit) once and if it is merged.