💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954757379)
It doesn't materially make a difference. Deriving a descriptor from either one will result in the same descriptor, or it's a bug.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954757379)
It doesn't materially make a difference. Deriving a descriptor from either one will result in the same descriptor, or it's a bug.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954758330)
Additionally there's memory management to worry about, if we really start pushing fresh template at every mempool increment, potentially multiple times per second.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954758330)
Additionally there's memory management to worry about, if we really start pushing fresh template at every mempool increment, potentially multiple times per second.
💬 ryanofsky commented on issue "doc/zmq: Note about endianness does not match reality":
(https://github.com/bitcoin/bitcoin/issues/31856#issuecomment-2657006696)
Agree with bug report and earlier comment. Relevant ZMQ code is [Lines 220 to 240 in zmqpublishnotifier.cpp](https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/src/zmq/zmqpublishnotifier.cpp#L220-L240) and the ZMQ documentation cited is pretty wrong or at least misleading:
> […] 32-byte hashes are in Little Endian and not in the Big Endian format that the RPC interface and block explorers use to display transaction and block hashes. [...]
>
> `| hashtx | <32-byte t
...
(https://github.com/bitcoin/bitcoin/issues/31856#issuecomment-2657006696)
Agree with bug report and earlier comment. Relevant ZMQ code is [Lines 220 to 240 in zmqpublishnotifier.cpp](https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/src/zmq/zmqpublishnotifier.cpp#L220-L240) and the ZMQ documentation cited is pretty wrong or at least misleading:
> […] 32-byte hashes are in Little Endian and not in the Big Endian format that the RPC interface and block explorers use to display transaction and block hashes. [...]
>
> `| hashtx | <32-byte t
...
📝 maflcko opened a pull request: " test: Rename send_message to send_without_ping "
(https://github.com/bitcoin/bitcoin/pull/31859)
`send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1950370246)
There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.
(https://github.com/bitcoin/bitcoin/pull/31859)
`send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1950370246)
There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.
💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954769058)
> First available number.
Oh, thanks. Didn't notice the numbers were out of order and later ones were used below.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954769058)
> First available number.
Oh, thanks. Didn't notice the numbers were out of order and later ones were used below.
💬 hodlinator commented on pull request "qa debug: Add --debug_runs/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2657022670)
fanquake:
> > Can you configure your IDE to do something like the equivalent of using `gdb`/`lldb`, and `process attach --name bitcoind --waitfor`? So it just grabs bitcoind once it spins up?
>
> I've never seen something like that in an IDE/editor, but I'll have a look, might exist a debugger console which allows that kind of thing.
The only debugging console I get with the IDE states "No Active Debug Session" when I try to input commands. :(
maflcko:
> > This is not about debugg
...
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2657022670)
fanquake:
> > Can you configure your IDE to do something like the equivalent of using `gdb`/`lldb`, and `process attach --name bitcoind --waitfor`? So it just grabs bitcoind once it spins up?
>
> I've never seen something like that in an IDE/editor, but I'll have a look, might exist a debugger console which allows that kind of thing.
The only debugging console I get with the IDE states "No Active Debug Session" when I try to input commands. :(
maflcko:
> > This is not about debugg
...
💬 hebasto commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657038440)
Concept ACK.
In addition to installation, components are also intended for use in packaging via [`cpack`](https://cmake.org/cmake/help/latest/manual/cpack.1.html). However, Bitcoin Core does not currently utilise it.
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657038440)
Concept ACK.
In addition to installation, components are also intended for use in packaging via [`cpack`](https://cmake.org/cmake/help/latest/manual/cpack.1.html). However, Bitcoin Core does not currently utilise it.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954786305)
Taken
`Assume(x)` is `x` in production, so in the event of a bug `tip_changed` will be false. So the function will just wait for the deadline and return nothing.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954786305)
Taken
`Assume(x)` is `x` in production, so in the event of a bug `tip_changed` will be false. So the function will just wait for the deadline and return nothing.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954786986)
I incorporated part of this comment. Note that some callers may just not be interested in fee changes, so I mentioned that as well.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954786986)
I incorporated part of this comment. Note that some callers may just not be interested in fee changes, so I mentioned that as well.
💬 maflcko commented on pull request "test: Rename send_message to send_without_ping":
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2657052286)
(Looks like there are 4 conflicts, so best to wait until there are 0. Otherwise there will be useless churn)
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2657052286)
(Looks like there are 4 conflicts, so best to wait until there are 0. Otherwise there will be useless churn)
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1954788703)
ah hm, maybe we don't need to wait? I guess if you send a ping, you don't get a pong until all 24 are processed.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1954788703)
ah hm, maybe we don't need to wait? I guess if you send a ping, you don't get a pong until all 24 are processed.
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954800384)
Up to this point you have not applied dependencies, so the new transaction is being considered its own singleton cluster, which is in-congruent with the actual goal. If you apply dependencies via CheckMemPoolPolicyLimits, then the resulting cluster is porentially oversized and violates the requirements set forth in the TxGraph interface.
see:
```
diff --git a/src/validation.cpp b/src/validation.cpp
index 61576851cd..affababd10 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@
...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954800384)
Up to this point you have not applied dependencies, so the new transaction is being considered its own singleton cluster, which is in-congruent with the actual goal. If you apply dependencies via CheckMemPoolPolicyLimits, then the resulting cluster is porentially oversized and violates the requirements set forth in the TxGraph interface.
see:
```
diff --git a/src/validation.cpp b/src/validation.cpp
index 61576851cd..affababd10 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@
...
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2657080863)
Can you update the PR description to say that this is based on #31854? The convention then is to mark this PR draft until that one is merged.
Additionally, it's 25% easier to review #31854 if you rebase this PR on the exact same commit. Since then I can just range-diff it with what I already reviewed here.
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2657080863)
Can you update the PR description to say that this is based on #31854? The convention then is to mark this PR draft until that one is merged.
Additionally, it's 25% easier to review #31854 if you rebase this PR on the exact same commit. Since then I can just range-diff it with what I already reviewed here.
💬 furszy commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657093782)
q: isn't the test just wrong?. `min_viable_change` is initialized to the future input fee without increasing the window by one as we do in the actual code. Thats why there is an overlap here. See the wallet code:
https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/src/wallet/spend.cpp#L1131-L1132
While you are manually setting it to:
```
coin_params.min_viable_change = coin_params.m_discard_feerate.GetFee(coin_params.change_spend_size);
```
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657093782)
q: isn't the test just wrong?. `min_viable_change` is initialized to the future input fee without increasing the window by one as we do in the actual code. Thats why there is an overlap here. See the wallet code:
https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/src/wallet/spend.cpp#L1131-L1132
While you are manually setting it to:
```
coin_params.min_viable_change = coin_params.m_discard_feerate.GetFee(coin_params.change_spend_size);
```
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954822133)
I think the mistake here is that in my call to `CountDistinctClusters()`, I should be setting `main_only` to true, and it looks like I omitted that so it'll call it on the staging graph instead. The same issue may apply with the invocation of `GetDescendants()`; the intent is to just call it on the main graph.
Would that resolve the concern you're bringing up, or is there something else I'm overlooking?
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954822133)
I think the mistake here is that in my call to `CountDistinctClusters()`, I should be setting `main_only` to true, and it looks like I omitted that so it'll call it on the staging graph instead. The same issue may apply with the invocation of `GetDescendants()`; the intent is to just call it on the main graph.
Would that resolve the concern you're bringing up, or is there something else I'm overlooking?
💬 marcofleon commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2657122152)
Thanks for adding that, I think it makes the script more complete. The only thing missing is the `runs=1` arg in `run_single` for when the entry is the full corpus dir. Right now it just runs continuously. But other than that, lgtm.
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2657122152)
Thanks for adding that, I think it makes the script more complete. The only thing missing is the `runs=1` arg in `run_single` for when the entry is the full corpus dir. Right now it just runs continuously. But other than that, lgtm.
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954834091)
> I should be setting main_only to true,
I think that works? Will think more on it.
> GetDescendants
Missed that one!
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954834091)
> I should be setting main_only to true,
I think that works? Will think more on it.
> GetDescendants
Missed that one!
💬 sr-gi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2657136142)
@tdb3 @danielabrozzoni can you give this another look? The changes should be trivial
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2657136142)
@tdb3 @danielabrozzoni can you give this another look? The changes should be trivial
💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954836649)
In commit "Have createNewBlock() wait for a tip" (854766b886b6c2ee564b867db64bdbce80064b3b)
Another alternative but probably not better is:
```diff
- if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};
+ if (!static_cast<Mining*>(this)->waitTipChanged(uint256::ZERO)) return {};
```
I think in longer term after we add an initial waitNext() method and make improvements to it, it will be natural to move more functionality out of Mining class methods i
...
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954836649)
In commit "Have createNewBlock() wait for a tip" (854766b886b6c2ee564b867db64bdbce80064b3b)
Another alternative but probably not better is:
```diff
- if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};
+ if (!static_cast<Mining*>(this)->waitTipChanged(uint256::ZERO)) return {};
```
I think in longer term after we add an initial waitNext() method and make improvements to it, it will be natural to move more functionality out of Mining class methods i
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2657139469)
(there seems to be a delay in Github processing my latest push)
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2657139469)
(there seems to be a delay in Github processing my latest push)