💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954742963)
The reason I kept it separate is because it's verbose. But happy to move it back.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954742963)
The reason I kept it separate is because it's verbose. But happy to move it back.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954755072)
A miner might naively set it so low that our p2p processing is impacted, potentially getting new blocks later and interfering with transaction relay.
Also, I hope to get rid of this soon(tm) if cluster mempool gives us a more efficient method. It's nice if we don't have to drop a configuration option again.
This is already 60x faster than the long poll behavior of `getblocktemplate`, which nobody complains about. And typical stratum (v1) jobs are refreshed every 30 seconds. https://b10c.me
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954755072)
A miner might naively set it so low that our p2p processing is impacted, potentially getting new blocks later and interfering with transaction relay.
Also, I hope to get rid of this soon(tm) if cluster mempool gives us a more efficient method. It's nice if we don't have to drop a configuration option again.
This is already 60x faster than the long poll behavior of `getblocktemplate`, which nobody complains about. And typical stratum (v1) jobs are refreshed every 30 seconds. https://b10c.me
...
💬 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