💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665909290)
> The burden to explain and motivate a change is on the pull request author. If there was a need to have changes without motivation, I am sure an LLM can generate them en mass.
The author was paged above and just thought he had justification or maybe it was just an oversight.
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665909290)
> The burden to explain and motivate a change is on the pull request author. If there was a need to have changes without motivation, I am sure an LLM can generate them en mass.
The author was paged above and just thought he had justification or maybe it was just an oversight.
👍 TheCharlatan approved a pull request: "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/31379#pullrequestreview-2623814063)
ACK c4c5cf174883cb53256e869f0d1673e29576a97c
I think I prefer this over passing through `SECP256K1_APPEND_FLAGS`. This is a distinct project after all, and having to configure all its subtrees/subprojects with separate flags seems cumbersome. It would also be inconsistent, since the method introduced here for libsecp already works fine for e.g. leveldb.
(https://github.com/bitcoin/bitcoin/pull/31379#pullrequestreview-2623814063)
ACK c4c5cf174883cb53256e869f0d1673e29576a97c
I think I prefer this over passing through `SECP256K1_APPEND_FLAGS`. This is a distinct project after all, and having to configure all its subtrees/subprojects with separate flags seems cumbersome. It would also be inconsistent, since the method introduced here for libsecp already works fine for e.g. leveldb.
💬 Sjors 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_r1959897031)
Added
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959897031)
Added
💬 Sjors 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_r1959897532)
I flipped it.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959897532)
I flipped it.
💬 Sjors 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#issuecomment-2665925797)
Addressed @vasild's feedback (and rebased).
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2665925797)
Addressed @vasild's feedback (and rebased).
💬 sipa commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665932107)
> The author was paged
You are the author of this PR. You should have a justification for having it open.
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665932107)
> The author was paged
You are the author of this PR. You should have a justification for having it open.
💬 sipa commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2665950978)
> would guess there is a livelock or deadlock or similar issue.
Is there progress at all? As in, is the number of validation blocks going not going up at all? That would be a bug. If it's just (very) slow, that may be due to low I/O speed of your disk + pruning + low dbcache. #28280 may speed things up somewhat (which will be in 29.0, not in any release yet).
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2665950978)
> would guess there is a livelock or deadlock or similar issue.
Is there progress at all? As in, is the number of validation blocks going not going up at all? That would be a bug. If it's just (very) slow, that may be due to low I/O speed of your disk + pruning + low dbcache. #28280 may speed things up somewhat (which will be in 29.0, not in any release yet).
💬 maflcko commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1959923000)
As the pull title is "CLI cleanups", just a random comment: It seems odd to treat `-getinfo` different than `-netinfo` (and everything else). It would be good for consistency to treat them the same:
```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index defe5e4c4e..18bd6b1f91 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1267,7 +1267,7 @@ static int CommandLineRPC(int argc, char *argv[])
gArgs.CheckMultipleCLIArgs();
std::unique_ptr<Ba
...
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1959923000)
As the pull title is "CLI cleanups", just a random comment: It seems odd to treat `-getinfo` different than `-netinfo` (and everything else). It would be good for consistency to treat them the same:
```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index defe5e4c4e..18bd6b1f91 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1267,7 +1267,7 @@ static int CommandLineRPC(int argc, char *argv[])
gArgs.CheckMultipleCLIArgs();
std::unique_ptr<Ba
...
💬 hebasto commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2665963150)
@purpleKarrot
> Concept NACK.
>
> I am not a big fan of simple utility functions that group a small set of builtin cmake commands. I think they increase the cognitive load and impede knowledge transfer from one cmake project to another. (I remember a time when builtin functions like `add_library` had to be wrapped and I authored many such wrappers, because there was no way of propagating interface requirements from one target to another. Those times are thankfully over since CMake version
...
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2665963150)
@purpleKarrot
> Concept NACK.
>
> I am not a big fan of simple utility functions that group a small set of builtin cmake commands. I think they increase the cognitive load and impede knowledge transfer from one cmake project to another. (I remember a time when builtin functions like `add_library` had to be wrapped and I authored many such wrappers, because there was no way of propagating interface requirements from one target to another. Those times are thankfully over since CMake version
...
💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665963156)
> You are the author of this PR. You should have a justification for having it open.
The justification is that the for loops are redundant and confusing for contributors trying to understand the test. There is no good reason to do such a thing is my justification for opening this PR.
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665963156)
> You are the author of this PR. You should have a justification for having it open.
The justification is that the for loops are redundant and confusing for contributors trying to understand the test. There is no good reason to do such a thing is my justification for opening this PR.
💬 hebasto commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1959928953)
@theuni
> Looking at this again, is there any reason [this needs to be an object library](https://github.com/bitcoin/bitcoin/blob/master/cmake/minisketch.cmake#L43)? It triggers the bug that necessitates the workaround here. I'm wondering if rather than using our hack or the (better) workaround suggested upstream, if we should just make it an archive library and avoid the issue altogether?
Please see https://github.com/bitcoin/bitcoin/pull/31880.
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1959928953)
@theuni
> Looking at this again, is there any reason [this needs to be an object library](https://github.com/bitcoin/bitcoin/blob/master/cmake/minisketch.cmake#L43)? It triggers the bug that necessitates the workaround here. I'm wondering if rather than using our hack or the (better) workaround suggested upstream, if we should just make it an archive library and avoid the issue altogether?
Please see https://github.com/bitcoin/bitcoin/pull/31880.
💬 purpleKarrot commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2665972886)
ACK. lgtm.
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2665972886)
ACK. lgtm.
💬 fjahr commented on issue "rpc: dumptxoutset as sqlite file":
(https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-2665978440)
I think this is resolved with the merged of #27432?
(https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-2665978440)
I think this is resolved with the merged of #27432?
🚀 fanquake merged a pull request: "test: remove scanning check on `wallet_importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/31893)
(https://github.com/bitcoin/bitcoin/pull/31893)
💬 hodlinator commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2666012863)
```
[shutoff] [...\httpserver.cpp:536] [StopHTTPServer] [http] Waiting for 1 connections to stop HTTP server
[http] [...\httpserver.cpp:355] [ThreadHTTP] [http] Exited http event loop
[net] [...\util\thread.cpp:22] [TraceThread] net thread exit
```
The fact that we don't get "Stopped HTTP server" or even "Waiting for HTTP event thread to exit" after these indicates that we are stuck in the lower part of `StopHTTPServer()`:
https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370f
...
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2666012863)
```
[shutoff] [...\httpserver.cpp:536] [StopHTTPServer] [http] Waiting for 1 connections to stop HTTP server
[http] [...\httpserver.cpp:355] [ThreadHTTP] [http] Exited http event loop
[net] [...\util\thread.cpp:22] [TraceThread] net thread exit
```
The fact that we don't get "Stopped HTTP server" or even "Waiting for HTTP event thread to exit" after these indicates that we are stuck in the lower part of `StopHTTPServer()`:
https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370f
...
💬 maflcko commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2666017905)
> Needs a fixup for MSAN fuzz.
What is the error?
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2666017905)
> Needs a fixup for MSAN fuzz.
What is the error?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959969112)
In commit "[bench] TxOrphanage::LimitOrphans"
Would it make sense to introduce this benchmark earlier (and the other ones below), so we can see what effect the previous commit has on it?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959969112)
In commit "[bench] TxOrphanage::LimitOrphans"
Would it make sense to introduce this benchmark earlier (and the other ones below), so we can see what effect the previous commit has on it?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959814286)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
Use `tx, std::move(announcer_list_pos), ...` to avoid an allocation.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959814286)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
Use `tx, std::move(announcer_list_pos), ...` to avoid an allocation.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959842788)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
Would it be possible to do the `list_pos` updating here without needing to return an iteration to push that responsibility to the caller? It would mean `RemoveIterAt` would need to know what peer it's operating in, so that means it's perhaps more appropriate to have it as ` TxOrphanage` member function rather than a `PeerOrphanInfo` member function.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959842788)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
Would it be possible to do the `list_pos` updating here without needing to return an iteration to push that responsibility to the caller? It would mean `RemoveIterAt` would need to know what peer it's operating in, so that means it's perhaps more appropriate to have it as ` TxOrphanage` member function rather than a `PeerOrphanInfo` member function.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959859482)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
I think this line, and the 7 lines before it, can be replaced with `RemoveIterAt(it->second.list_pos)`, especially if it can be changed to do the `list_pos` updating internally?
Not very important as the code disappears in the next commit, but would make it more obviously correct.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1959859482)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
I think this line, and the 7 lines before it, can be replaced with `RemoveIterAt(it->second.list_pos)`, especially if it can be changed to do the `list_pos` updating internally?
Not very important as the code disappears in the next commit, but would make it more obviously correct.