💬 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.
✅ hebasto closed an issue: "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable""
(https://github.com/bitcoin/bitcoin/issues/31881)
(https://github.com/bitcoin/bitcoin/issues/31881)
💬 hebasto commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2666041425)
Resolved in https://github.com/bitcoin/bitcoin/pull/31893.
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2666041425)
Resolved in https://github.com/bitcoin/bitcoin/pull/31893.
📝 maflcko opened a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896)
`IsArgSet` is problematic:
* It returns whether an arg has been set, even if it has been negated. `IsArgSet` is sometimes used to check for a truthy value, which is wrong, but usually harmless. Cleanup of those cases may or may not be done in a follow-up.
* In most other cases, calling it is redundant, because the immediately following `Get*Arg` calls can already return an `std::optional` nullopt value to indicate an unset arg.
So relieve both issues by removing all `IsArgSet` that are re
...
(https://github.com/bitcoin/bitcoin/pull/31896)
`IsArgSet` is problematic:
* It returns whether an arg has been set, even if it has been negated. `IsArgSet` is sometimes used to check for a truthy value, which is wrong, but usually harmless. Cleanup of those cases may or may not be done in a follow-up.
* In most other cases, calling it is redundant, because the immediately following `Get*Arg` calls can already return an `std::optional` nullopt value to indicate an unset arg.
So relieve both issues by removing all `IsArgSet` that are re
...
💬 maflcko commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959977366)
nit: I find the `!Value(gArgs).isNull()` with the double negation and verbosity over just `Get(gArgs)` a bit ugly and I think it would be better to remove `IsArgSet` as much as possible. In most cases it is not needed anyway, or used incorrectly. About this one, I left a comment here: https://github.com/bitcoin/bitcoin/pull/31887/files#r1959923000
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959977366)
nit: I find the `!Value(gArgs).isNull()` with the double negation and verbosity over just `Get(gArgs)` a bit ugly and I think it would be better to remove `IsArgSet` as much as possible. In most cases it is not needed anyway, or used incorrectly. About this one, I left a comment here: https://github.com/bitcoin/bitcoin/pull/31887/files#r1959923000
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959978900)
Agree on double-quotes in more places giving nicer output, but would not rephrase or remove legacy wallets at this point.
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the \"wallets\" directory), or a legacy wallet's .dat file (within the \"wallets\" directory). The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```
I
...
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959978900)
Agree on double-quotes in more places giving nicer output, but would not rephrase or remove legacy wallets at this point.
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the \"wallets\" directory), or a legacy wallet's .dat file (within the \"wallets\" directory). The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```
I
...
📝 Sjors opened a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897)
For the coinbase `vTxFees` used a dummy value of -nFees.
Similarly the first `vTxSigOpsCost` entry was calculated from
the dummy coinbase transaction.
This was introduced in #2115, but the values were never returned by the RPC or used in a test.
Set both to 0 instead and add code comments to prevent confusion.
(https://github.com/bitcoin/bitcoin/pull/31897)
For the coinbase `vTxFees` used a dummy value of -nFees.
Similarly the first `vTxSigOpsCost` entry was calculated from
the dummy coinbase transaction.
This was introduced in #2115, but the values were never returned by the RPC or used in a test.
Set both to 0 instead and add code comments to prevent confusion.
💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960001093)
I think invalidating ACKs is more a consideration for critical or difficult-to-review changes.
If loading legacy wallets will be disabled in the next release, it may be worth not adding a mention here that would require attention/removal.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960001093)
I think invalidating ACKs is more a consideration for critical or difficult-to-review changes.
If loading legacy wallets will be disabled in the next release, it may be worth not adding a mention here that would require attention/removal.
🚀 fanquake merged a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892)
(https://github.com/bitcoin/bitcoin/pull/31892)
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960003712)
I opened #31897 to drop this `-nFees` dummy.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960003712)
I opened #31897 to drop this `-nFees` dummy.
💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960005999)
> Agree on double-quotes in more places giving nicer output
The suggested diff doesn't only add double quotes but also proposes a rewrite for clarity between wallet directory and wallets directory.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960005999)
> Agree on double-quotes in more places giving nicer output
The suggested diff doesn't only add double quotes but also proposes a rewrite for clarity between wallet directory and wallets directory.