π¬ romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586262733)
Thanks!
https://github.com/bitcoin/bitcoin/compare/f07c765aa6bdba2511ceec56aa7f9755fa29a81e..d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586262733)
Thanks!
https://github.com/bitcoin/bitcoin/compare/f07c765aa6bdba2511ceec56aa7f9755fa29a81e..d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7
π¬ 0xB10C commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3608377779)
> Ignoring self-announcements seems to occur only in the time between our inbound peer (outbound from their perspective) 1) calling `SetupAddressRelay` when receiving version, and 2) receiving verack where it will set `fSuccessfullyConnected = true` and call `SendMessages` followed by `MaybeSendAddr`. The address entries are sent in order of insert, but then shuffled before processing since #22387 ([see rationale](https://github.com/bitcoin/bitcoin/pull/22387#discussion_r664238361)). If they wer
...
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3608377779)
> Ignoring self-announcements seems to occur only in the time between our inbound peer (outbound from their perspective) 1) calling `SetupAddressRelay` when receiving version, and 2) receiving verack where it will set `fSuccessfullyConnected = true` and call `SendMessages` followed by `MaybeSendAddr`. The address entries are sent in order of insert, but then shuffled before processing since #22387 ([see rationale](https://github.com/bitcoin/bitcoin/pull/22387#discussion_r664238361)). If they wer
...
π€ marcofleon reviewed a pull request: "Replace cluster linearization algorithm with SFL"
(https://github.com/bitcoin/bitcoin/pull/32545#pullrequestreview-3536407029)
Did a first pass of the changes in `cluster_linearize.h`, left a couple small comments. I'm running a few of the fuzz targets, including the new one, and I'll leave those going for a while.
Still need to look at the tests thoroughly. Let me know if there's anything specfic you think reviewers can do that would be useful.
(https://github.com/bitcoin/bitcoin/pull/32545#pullrequestreview-3536407029)
Did a first pass of the changes in `cluster_linearize.h`, left a couple small comments. I'm running a few of the fuzz targets, including the new one, and I'll leave those going for a while.
Still need to look at the tests thoroughly. Let me know if there's anything specfic you think reviewers can do that would be useful.
π¬ marcofleon commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2586214542)
```suggestion
SetInfo operator-(const SetInfo& other) const noexcept
```
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2586214542)
```suggestion
SetInfo operator-(const SetInfo& other) const noexcept
```
π¬ marcofleon commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2586249205)
nit: Is there a reason we recalculate here vs just using `chunk_rep` from the line above?
Also additional nit: The local `chunk_rep` is named the same as the function parameter and both are `TxIdx` I believe. Could be worth having different names?
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2586249205)
nit: Is there a reason we recalculate here vs just using `chunk_rep` from the line above?
Also additional nit: The local `chunk_rep` is named the same as the function parameter and both are `TxIdx` I believe. Could be worth having different names?
π hodlinator approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536580365)
re-ACK d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536580365)
re-ACK d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7
π¬ maflcko commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586340053)
> in an `if` block above that doesn't do anything else now seemed unnecessary to me.
What the if-else block does is explained right after the `if`:
```cpp
if (auto mempool_tx = node.mempool->get(txid); mempool_tx) {
// There's already a transaction in the mempool with this txid.
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586340053)
> in an `if` block above that doesn't do anything else now seemed unnecessary to me.
What the if-else block does is explained right after the `if`:
```cpp
if (auto mempool_tx = node.mempool->get(txid); mempool_tx) {
// There's already a transaction in the mempool with this txid.
π¬ rkrux commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586371586)
Ah, there's another `if` above on line 66: https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/node/transaction.cpp#L66
I had missed that, good catch. I see now what you mean.
Setting `callback_set` in the `else` block seems reasonable. This suggestion can be reverted: https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2571218739.
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586371586)
Ah, there's another `if` above on line 66: https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/node/transaction.cpp#L66
I had missed that, good catch. I see now what you mean.
Setting `callback_set` in the `else` block seems reasonable. This suggestion can be reverted: https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2571218739.
π¬ marcofleon commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3608532244)
> I donβt think this is an issue. Youβre just massively oversubscribing the CPU and lowering the timeout to the point where all the context switching triggers it. Switching to notify_all() only forces all workers awake on every submission, which masks the OS scheduler starvation you get in this kind of extreme setup.
I've been playing with the fuzz test and found that this fixes it for me:
```c++
ThreadPool g_pool{"fuzz"};
size_t g_num_workers = 3;
std::atomic<bool> g_pool_started{false
...
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3608532244)
> I donβt think this is an issue. Youβre just massively oversubscribing the CPU and lowering the timeout to the point where all the context switching triggers it. Switching to notify_all() only forces all workers awake on every submission, which masks the OS scheduler starvation you get in this kind of extreme setup.
I've been playing with the fuzz test and found that this fixes it for me:
```c++
ThreadPool g_pool{"fuzz"};
size_t g_num_workers = 3;
std::atomic<bool> g_pool_started{false
...
π sedited approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536659569)
Re-ACK d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536659569)
Re-ACK d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7
π¬ maflcko commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3608602721)
> Open/closed to re-run CI, and it seems to have the same issue.
Is it fixed now?
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3608602721)
> Open/closed to re-run CI, and it seems to have the same issue.
Is it fixed now?
π¬ maflcko commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-3608605508)
Could turn into draft, while ci is red?
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-3608605508)
Could turn into draft, while ci is red?
π¬ darosior commented on issue "ASN-based bucketing of the network nodes":
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-3608666477)
Thanks for the data @virtu. I think it's important to make a compelling case that switching to ASmap by default won't lead to a degraded network that would take a long time to fix due to the slow take-up on new releases.
Regarding your minor concern, i previously [asked](https://btctranscripts.com/bitcoin-core-dev-tech/2025-02/asmap) about this and got back that the half-life of a map should be around 5 years. So it should always be better than the current method. And i guess it's always possib
...
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-3608666477)
Thanks for the data @virtu. I think it's important to make a compelling case that switching to ASmap by default won't lead to a degraded network that would take a long time to fix due to the slow take-up on new releases.
Regarding your minor concern, i previously [asked](https://btctranscripts.com/bitcoin-core-dev-tech/2025-02/asmap) about this and got back that the half-life of a map should be around 5 years. So it should always be better than the current method. And i guess it's always possib
...
β οΈ Sreejitroy opened an issue: "Request to help mining two Nonstandard UTXO which fails cleanstack"
(https://github.com/bitcoin/bitcoin/issues/34002)
### Please describe the feature you'd like to see added.
I have 2 anybody can spend UTXO which fails cleanstack validation which says non-mandatory script verify flag(Stack size must be exactly one after execution) for P2SH, the script passes consensus perfectly as top item is true but get problem here even if it ends with 2 elements gives the same error. Requesting you to at least make it 5 elements max on the stack .
### Is your feature related to a problem, if so please describe it.
_No re
...
(https://github.com/bitcoin/bitcoin/issues/34002)
### Please describe the feature you'd like to see added.
I have 2 anybody can spend UTXO which fails cleanstack validation which says non-mandatory script verify flag(Stack size must be exactly one after execution) for P2SH, the script passes consensus perfectly as top item is true but get problem here even if it ends with 2 elements gives the same error. Requesting you to at least make it 5 elements max on the stack .
### Is your feature related to a problem, if so please describe it.
_No re
...
π¬ romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586531856)
Split into 3 commits:
https://github.com/bitcoin/bitcoin/compare/d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7..e6eb4a57df9f0379550358e4362729bfe5953007
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586531856)
Split into 3 commits:
https://github.com/bitcoin/bitcoin/compare/d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7..e6eb4a57df9f0379550358e4362729bfe5953007
π¬ l0rinc commented on pull request "qa: Avoid knock-on exception in assert_start_raises_init_error":
(https://github.com/bitcoin/bitcoin/pull/32929#issuecomment-3608760754)
I have tested the previous ACK, now just reviewed the range-diff and rebased soft-reset, the latest change looks good to me.
ACK 6b0eac750f3aecb29446d70c039559143e43a011
<details>
<summary>patch since last ACK</summary>
```patch
diff --git a/test/functional/feature_framework_startup_failures.py b/test/functional/feature_framework_startup_failures.py
--- a/test/functional/feature_framework_startup_failures.py (revision 298789b8cbc7283fd868934c615390b98943ebba)
+++ b/test/functional/f
...
(https://github.com/bitcoin/bitcoin/pull/32929#issuecomment-3608760754)
I have tested the previous ACK, now just reviewed the range-diff and rebased soft-reset, the latest change looks good to me.
ACK 6b0eac750f3aecb29446d70c039559143e43a011
<details>
<summary>patch since last ACK</summary>
```patch
diff --git a/test/functional/feature_framework_startup_failures.py b/test/functional/feature_framework_startup_failures.py
--- a/test/functional/feature_framework_startup_failures.py (revision 298789b8cbc7283fd868934c615390b98943ebba)
+++ b/test/functional/f
...
π¬ hodlinator commented on pull request "qa: Avoid knock-on exception in assert_start_raises_init_error":
(https://github.com/bitcoin/bitcoin/pull/32929#issuecomment-3608761603)
Latest push (6b0eac750f3aecb29446d70c039559143e43a011):
* Restores the " (cmd: ..." addition in d7ff1d0fa572ec36ff1eb10c0a1c04ef5e5be072 which had somehow gone missing in 4eccfae4563933c8499dc6994980d8bb8df53381. The test in c06a8ee18699800a0fa06898251ac7088ea8ef70 had also lost " \(cmd:" in ae69cf7bc7ebecd0d58b007a2c556a787554af28, so it's not just a mismerge. Possibly a case of working on multiple computers and not syncing properly between them before force-pushing. Thanks to @l0rinc for find
...
(https://github.com/bitcoin/bitcoin/pull/32929#issuecomment-3608761603)
Latest push (6b0eac750f3aecb29446d70c039559143e43a011):
* Restores the " (cmd: ..." addition in d7ff1d0fa572ec36ff1eb10c0a1c04ef5e5be072 which had somehow gone missing in 4eccfae4563933c8499dc6994980d8bb8df53381. The test in c06a8ee18699800a0fa06898251ac7088ea8ef70 had also lost " \(cmd:" in ae69cf7bc7ebecd0d58b007a2c556a787554af28, so it's not just a mismerge. Possibly a case of working on multiple computers and not syncing properly between them before force-pushing. Thanks to @l0rinc for find
...
π sedited approved a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3536863021)
ACK d9319b06cf82664d55f255387a348135fd7f91c7
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3536863021)
ACK d9319b06cf82664d55f255387a348135fd7f91c7
π¬ romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3608793203)
Split https://github.com/bitcoin/bitcoin/commit/d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7 into 3 commits:
$ git lg -3
* e6eb4a57df Roman Zeyde: rest: allow reading partial block data from storage
* 8f0bfb9343 Roman Zeyde: blockstorage: allow reading partial block data from storage
* 07b8278299 Roman Zeyde: blockstorage: overload `ReadRawBlock()` to return an error code
https://github.com/bitcoin/bitcoin/compare/d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7..e6eb4a57df9f0379550358e4362729bf
...
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3608793203)
Split https://github.com/bitcoin/bitcoin/commit/d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7 into 3 commits:
$ git lg -3
* e6eb4a57df Roman Zeyde: rest: allow reading partial block data from storage
* 8f0bfb9343 Roman Zeyde: blockstorage: allow reading partial block data from storage
* 07b8278299 Roman Zeyde: blockstorage: overload `ReadRawBlock()` to return an error code
https://github.com/bitcoin/bitcoin/compare/d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7..e6eb4a57df9f0379550358e4362729bf
...
π sedited approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536915373)
Re-ACK e6eb4a57df9f0379550358e4362729bfe5953007
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536915373)
Re-ACK e6eb4a57df9f0379550358e4362729bfe5953007