🤔 sipa reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1853774778)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1853774778)
Concept ACK
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472849582)
Did you mean "then EXPLORE" here?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472849582)
Did you mean "then EXPLORE" here?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472848173)
I think you should set the number of tries to be at least some multiple of the utxo_pool size, because (a) without it, you can't guarantee even considering every UTXO, and (b) the runtime of coin selection overall already contains a component proportional to the size of utxo_pool (simply the cost of constructing the pool in the first place) anyway. So I would suggest something like `const auto total_tries = max(TOTAL_TRIES_CG, 10 * utxo_pool.size());` or so.
It may be worth doing this as a fo
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472848173)
I think you should set the number of tries to be at least some multiple of the utxo_pool size, because (a) without it, you can't guarantee even considering every UTXO, and (b) the runtime of coin selection overall already contains a component proportional to the size of utxo_pool (simply the cost of constructing the pool in the first place) anyway. So I would suggest something like `const auto total_tries = max(TOTAL_TRIES_CG, 10 * utxo_pool.size());` or so.
It may be worth doing this as a fo
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472865513)
Coding style nit:
```c++
} else {
append_error(cg_result);
}
```
(single-line conditional only allowed if it's of the `if (condition) statement;` form)
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472865513)
Coding style nit:
```c++
} else {
append_error(cg_result);
}
```
(single-line conditional only allowed if it's of the `if (condition) statement;` form)
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472864023)
This `result.SetSelectionsEvaluated(curr_try); break;` is repeated in both exit conditions. The `result.SetSelectionsEvaluated(curr_try);` can be moved outside of the loop I believe.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472864023)
This `result.SetSelectionsEvaluated(curr_try); break;` is repeated in both exit conditions. The `result.SetSelectionsEvaluated(curr_try);` can be moved outside of the loop I believe.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472986777)
I've experimented a bit with how far the range of inputs can be extended to cover extreme values (not because those extreme values are ones we care about in practice, but because they're more likely to expose bugs that do affect practical inputs):
```diff
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -142,29 +142,25 @@ FUZZ_TARGET(coin_grinder_is_optimal)
FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)};
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472986777)
I've experimented a bit with how far the range of inputs can be extended to cover extreme values (not because those extreme values are ones we care about in practice, but because they're more likely to expose bugs that do affect practical inputs):
```diff
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -142,29 +142,25 @@ FUZZ_TARGET(coin_grinder_is_optimal)
FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)};
...
💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#discussion_r1473024162)
thanks, renamed to `connection_opened`
(https://github.com/bitcoin/bitcoin/pull/29352#discussion_r1473024162)
thanks, renamed to `connection_opened`
💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919339509)
Fixed both issues, thanks!
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919339509)
Fixed both issues, thanks!
🤔 ryanofsky reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1853918541)
Code review 71059cf4267d80d2934b85a7046fc7b64900378b
I'm starting to review this, but I don't understand how it interacts with abandoned transactions. Is it still possible to abandon a transaction with conflicts with the mempool? If a transaction is already abandoned and we know it conflicts with the mempool does isMempoolConflicted still return false?
It seems to me like a more straightforward way to implement this feature would be to add a `bool mempool_conflicts` field to `TxStateInacti
...
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1853918541)
Code review 71059cf4267d80d2934b85a7046fc7b64900378b
I'm starting to review this, but I don't understand how it interacts with abandoned transactions. Is it still possible to abandon a transaction with conflicts with the mempool? If a transaction is already abandoned and we know it conflicts with the mempool does isMempoolConflicted still return false?
It seems to me like a more straightforward way to implement this feature would be to add a `bool mempool_conflicts` field to `TxStateInacti
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472930692)
In commit "wallet: use CWalletTx member functions to determine tx state" (86aaf81f81b48e82358db6700a6c9d46c5b5b0df)
Code comment here is just repeating the boolean expression and not explaining anything. I think the simplest explanation of this is "An output is considered spent by a spending transaction, unless the spending transaction is conflicted or abandoned." Could also add "But the check below is written to check for valid spending transactions instead of invalid ones so it isn't tied t
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472930692)
In commit "wallet: use CWalletTx member functions to determine tx state" (86aaf81f81b48e82358db6700a6c9d46c5b5b0df)
Code comment here is just repeating the boolean expression and not explaining anything. I think the simplest explanation of this is "An output is considered spent by a spending transaction, unless the spending transaction is conflicted or abandoned." Could also add "But the check below is written to check for valid spending transactions instead of invalid ones so it isn't tied t
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472955815)
In commit "wallet: track mempool conflicts" (a6ae5b23b1497ab6f4899db49348db623700a2d8)
I think it would be useful to add that mempool conflicted transactions are serialized as inactive transactions, so the distinction between TxStateMempoolConflicted and TxStateInactive only exists at runtime.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472955815)
In commit "wallet: track mempool conflicts" (a6ae5b23b1497ab6f4899db49348db623700a2d8)
I think it would be useful to add that mempool conflicted transactions are serialized as inactive transactions, so the distinction between TxStateMempoolConflicted and TxStateInactive only exists at runtime.
🤔 sr-gi reviewed a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854149947)
I wonder if we will not better off not having a simpler `initiate_v2_handshake` in this case and avoiding sync + async logic. Given we end up having to manually send raw messages and accessing `peer1` internal state anyway, I think it would be simpler to send everything manually:
```python
class TestEncryptedP2PState(EncryptedP2PState):
def __init__(self):
super().__init__(initiating=True, net='regtest')
self.magic_sent = False
def initiate_v2_handshake(self)
...
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854149947)
I wonder if we will not better off not having a simpler `initiate_v2_handshake` in this case and avoiding sync + async logic. Given we end up having to manually send raw messages and accessing `peer1` internal state anyway, I think it would be simpler to send everything manually:
```python
class TestEncryptedP2PState(EncryptedP2PState):
def __init__(self):
super().__init__(initiating=True, net='regtest')
self.magic_sent = False
def initiate_v2_handshake(self)
...
👍 theStack approved a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854169206)
Tested ACK 9642aefb81a9c87227eccf9997380024247ed1fc
Reproduced the issue and verified it's fix by adding an artificial delay in `initiate_v2_handshake`, as suggested in the PR description.
(With artificial delays of 3 seconds or more, the test fails again with an `IOError('Not connected')`, but this is expected as the TestNode closes the connection if there is no activity: `... [InactivityCheck] [net] socket no message in first 3 seconds ...`)
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854169206)
Tested ACK 9642aefb81a9c87227eccf9997380024247ed1fc
Reproduced the issue and verified it's fix by adding an artificial delay in `initiate_v2_handshake`, as suggested in the PR description.
(With artificial delays of 3 seconds or more, the test fails again with an `IOError('Not connected')`, but this is expected as the TestNode closes the connection if there is no activity: `... [InactivityCheck] [net] socket no message in first 3 seconds ...`)
🤔 furszy reviewed a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692#pullrequestreview-1852779591)
Executing `generate 1` on a node without a wallet leads to a program crash. And left some other review notes.
Will read the historical context for the `generate` command deletion before proceed.
(https://github.com/bitcoin-core/gui/pull/692#pullrequestreview-1852779591)
Executing `generate 1` on a node without a wallet leads to a program crash. And left some other review notes.
Will read the historical context for the `generate` command deletion before proceed.
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473018733)
The function iterates over the entire input command string twice. It isn't really "fast".
Could remove this line altogether.
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473018733)
The function iterates over the entire input command string twice. It isn't really "fast".
Could remove this line altogether.
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473029584)
There is an indentation issue here.
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473029584)
There is an indentation issue here.
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1472227098)
This should pass through the model.
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1472227098)
This should pass through the model.
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473064260)
In case you retouch this; these functions should return a `util::Result<QString>` instead of a boolean. This way, the returned string can be emitted at the caller side (same as it is done for the RPC commands). This will improve the existing structure that has all functions returning true on all code paths.
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473064260)
In case you retouch this; these functions should return a `util::Result<QString>` instead of a boolean. This way, the returned string can be emitted at the caller side (same as it is done for the RPC commands). This will improve the existing structure that has all functions returning true on all code paths.
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473055299)
Same as above, this shouldn't be here. It is explaining the caller's workflow inside `executeConsoleGenerate`. There is no other RPC calls at this point.
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473055299)
Same as above, this shouldn't be here. It is explaining the caller's workflow inside `executeConsoleGenerate`. There is no other RPC calls at this point.
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473035544)
This shouldn't be here. The function's docstrings shouldn't explain the caller workflow behavior.
Something like "Executes gui-console-only commands" would be preferable.
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473035544)
This shouldn't be here. The function's docstrings shouldn't explain the caller workflow behavior.
Something like "Executes gui-console-only commands" would be preferable.