💬 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.
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473052359)
To access the chain type, use the node interface. See [03d67301e081ecf3123372901b115ee5e29d7c79](https://github.com/furszy/bitcoin-core/commit/03d67301e081ecf3123372901b115ee5e29d7c79). So we don't violate the layers division.
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473052359)
To access the chain type, use the node interface. See [03d67301e081ecf3123372901b115ee5e29d7c79](https://github.com/furszy/bitcoin-core/commit/03d67301e081ecf3123372901b115ee5e29d7c79). So we don't violate the layers division.
💬 murchandamus commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1919410802)
Hey, I was taking a look at this PR. What is the status on this? Are we aiming to get this into 27.0? What about the two PRs, that @S3RK mentioned, they are open and in draft. Does this PR here depend on them?
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1919410802)
Hey, I was taking a look at this PR. What is the status on this? Are we aiming to get this into 27.0? What about the two PRs, that @S3RK mentioned, they are open and in draft. Does this PR here depend on them?
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1473075595)
I'm not sure if this should be optional. Was thinking before about asserting here. Wdyt?
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1473075595)
I'm not sure if this should be optional. Was thinking before about asserting here. Wdyt?
💬 epiccurious commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1919414747)
utACK
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1919414747)
utACK
💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919419940)
> I wonder if we will not better off 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:
Sounds reasonable to me, but I'd like to refer that to @stratospher, who I believe has plans to extend / rewrite the test anyway (see the WIP branch linked [here](https://github.com/bitcoin/bitcoin/pull/24748#issuecomm
...
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919419940)
> I wonder if we will not better off 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:
Sounds reasonable to me, but I'd like to refer that to @stratospher, who I believe has plans to extend / rewrite the test anyway (see the WIP branch linked [here](https://github.com/bitcoin/bitcoin/pull/24748#issuecomm
...