💬 sr-gi commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598815010)
In c1be7952cff9669b87e5002dcbc990dbf726c06f
Given this is called right after `QueryDNSSeeds()` conditionally to `-dnsseed` being set, we could make this method take it instead of re-parsing it here.
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598815010)
In c1be7952cff9669b87e5002dcbc990dbf726c06f
Given this is called right after `QueryDNSSeeds()` conditionally to `-dnsseed` being set, we could make this method take it instead of re-parsing it here.
💬 sr-gi commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598833951)
In c1be7952cff9669b87e5002dcbc990dbf726c06f
This can also be passed as a param. We are parsing it both here and in `CConnman::QueryDNSSeeds()`
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598833951)
In c1be7952cff9669b87e5002dcbc990dbf726c06f
This can also be passed as a param. We are parsing it both here and in `CConnman::QueryDNSSeeds()`
💬 sr-gi commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598838227)
In c1be7952cff9669b87e5002dcbc990dbf726c06f
A timer is used both for `QueryDNSSeeds` and `ProcessFixedSeeds` (to wait for 30 secs on the former, and 1 min on the latter). The timer is implicitly shared, we are setting it before entering `QueryDNSSeeds` and passing it to `ProcessFixedSeeds`. However, for `QueryDNSSeeds` we parse the time again inside the method. This should not be necessary. `start_time` could also be passed here.
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598838227)
In c1be7952cff9669b87e5002dcbc990dbf726c06f
A timer is used both for `QueryDNSSeeds` and `ProcessFixedSeeds` (to wait for 30 secs on the former, and 1 min on the latter). The timer is implicitly shared, we are setting it before entering `QueryDNSSeeds` and passing it to `ProcessFixedSeeds`. However, for `QueryDNSSeeds` we parse the time again inside the method. This should not be necessary. `start_time` could also be passed here.
📝 theuni opened a pull request: "crypto: disable asan for sha256_sse4 with clang and -O0"
(https://github.com/bitcoin/bitcoin/pull/30097)
Clang is unable to compile the Transform function for that combination of options.
Fixes #29801.
(https://github.com/bitcoin/bitcoin/pull/30097)
Clang is unable to compile the Transform function for that combination of options.
Fixes #29801.
💬 theuni commented on issue "Compilation failure with `-O0` + `-fsanitize=address` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2108512757)
@ajtowns Thanks for the reminder here. See #30097 for a function-level application of the above.
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2108512757)
@ajtowns Thanks for the reminder here. See #30097 for a function-level application of the above.
💬 theuni commented on pull request "crypto: disable asan for sha256_sse4 with clang and -O0":
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2108514077)
Ping @achow101. Sorry this took so long!
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2108514077)
Ping @achow101. Sorry this took so long!
💬 edilmedeiros commented on issue "contrib/signet/miner: miner won't consider --nbits parameter":
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2108523883)
@ajtowns Thanks for the explanation, the miner behavior makes a lot more sense now.
I did a "quick" experiment this morning: started a new signet with the first block starting a month ago. My CPU went close to 100% using all available cores.
<details>
<summary>At first, it was mining a few blocks a second.</summary>
```
2024-05-13 13:06:47 INFO Mined block at height 2; next in -717h26m32s (mine)
2024-05-13 13:06:47 INFO Mined block at height 3; next in -717h24m2s (mine)
2024-05-13
...
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2108523883)
@ajtowns Thanks for the explanation, the miner behavior makes a lot more sense now.
I did a "quick" experiment this morning: started a new signet with the first block starting a month ago. My CPU went close to 100% using all available cores.
<details>
<summary>At first, it was mining a few blocks a second.</summary>
```
2024-05-13 13:06:47 INFO Mined block at height 2; next in -717h26m32s (mine)
2024-05-13 13:06:47 INFO Mined block at height 3; next in -717h24m2s (mine)
2024-05-13
...
💬 ryanofsky commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652)
re: https://github.com/bitcoin/bitcoin/pull/29346#issue-2105856059
> We could decide to not support Noise encryption at all and require users to install separate software for that. Code review of this PR could help inform that decision.
I'm just starting to look at this PR and #29432, and having separate software to handle the encryption and networking does seem like a pretty appealing approach.
However, even if the goal is to directly add support for the stratum protocol to bitcoin cor
...
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652)
re: https://github.com/bitcoin/bitcoin/pull/29346#issue-2105856059
> We could decide to not support Noise encryption at all and require users to install separate software for that. Code review of this PR could help inform that decision.
I'm just starting to look at this PR and #29432, and having separate software to handle the encryption and networking does seem like a pretty appealing approach.
However, even if the goal is to directly add support for the stratum protocol to bitcoin cor
...
💬 hebasto commented on issue "make cov fails with lcov-2":
(https://github.com/bitcoin/bitcoin/issues/28468#issuecomment-2108530787)
> > Mind sharing it?
>
> https://github.com/fanquake/bitcoin/tree/support_lcov_2.
To work without `--enable-lcov-branch-coverage`, it should be:
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -842,12 +842,14 @@ if test "$use_lcov" = "yes"; then
[AC_MSG_ERROR([lcov testing requested but --coverage linker flag does not work])])
AX_CHECK_COMPILE_FLAG([--coverage],[CORE_CXXFLAGS="$CORE_CXXFLAGS --coverage"],
[AC_MSG_ERROR([lcov testing requested but --coverage flag does n
...
(https://github.com/bitcoin/bitcoin/issues/28468#issuecomment-2108530787)
> > Mind sharing it?
>
> https://github.com/fanquake/bitcoin/tree/support_lcov_2.
To work without `--enable-lcov-branch-coverage`, it should be:
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -842,12 +842,14 @@ if test "$use_lcov" = "yes"; then
[AC_MSG_ERROR([lcov testing requested but --coverage linker flag does not work])])
AX_CHECK_COMPILE_FLAG([--coverage],[CORE_CXXFLAGS="$CORE_CXXFLAGS --coverage"],
[AC_MSG_ERROR([lcov testing requested but --coverage flag does n
...
💬 Roasbeef commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1598894712)
Wouldn't it be better to _defer_ the creation of the genesis block until a much farther date, so commit to changing during the rc phase, until the final release. We'd then include a similar mainnet block height hash or w/e as well. This way people aren't mining the chain for months before it's included in a real release.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1598894712)
Wouldn't it be better to _defer_ the creation of the genesis block until a much farther date, so commit to changing during the rc phase, until the final release. We'd then include a similar mainnet block height hash or w/e as well. This way people aren't mining the chain for months before it's included in a real release.
👍 BrandonOdiwuor approved a pull request: "rpc: move UniValue in blockToJSON"
(https://github.com/bitcoin/bitcoin/pull/30094#pullrequestreview-2053443097)
ACK b77bad309e92f176f340598eec056eb7bff86f5f
(https://github.com/bitcoin/bitcoin/pull/30094#pullrequestreview-2053443097)
ACK b77bad309e92f176f340598eec056eb7bff86f5f
💬 willcl-ark commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108560277)
> Unrelated to the blockToJSON changes here, as a follow-up, it may be possible to "fix" #25229 in the same way?
I can push a similar commit here if it's worth merging the two. Or open a separate PR? Perhaps with 3 ACKs a separate PR is preferable? I also am only halfway through making a "big wallet" (using achow101's [make-big-wallet](https://gist.github.com/achow101/f24309bba501cc08c9a3a3d55fa6eec6) script, so I have no suitable wallet to test such a commit against currently.
> Actually
...
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108560277)
> Unrelated to the blockToJSON changes here, as a follow-up, it may be possible to "fix" #25229 in the same way?
I can push a similar commit here if it's worth merging the two. Or open a separate PR? Perhaps with 3 ACKs a separate PR is preferable? I also am only halfway through making a "big wallet" (using achow101's [make-big-wallet](https://gist.github.com/achow101/f24309bba501cc08c9a3a3d55fa6eec6) script, so I have no suitable wallet to test such a commit against currently.
> Actually
...
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598896733)
Oh but I don't want to use cut-through; that can only be compared when I generate the index at exactly the same height.
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598896733)
Oh but I don't want to use cut-through; that can only be compared when I generate the index at exactly the same height.
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108563853)
> the stratum code would call a more well-defined interface
I would like that too. Just not sure how to go about it.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108563853)
> the stratum code would call a more well-defined interface
I would like that too. Just not sure how to go about it.
💬 theuni commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108563958)
> > Actually, looks like this could use a txs.reserve(block.vtx.size()) as well.
>
> I don't see `reserve` on `UniValue`. Perhaps I a missing what you mean here?
Whoops, of course you're right. It probably should though :)
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108563958)
> > Actually, looks like this could use a txs.reserve(block.vtx.size()) as well.
>
> I don't see `reserve` on `UniValue`. Perhaps I a missing what you mean here?
Whoops, of course you're right. It probably should though :)
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2108566561)
> The benefits I see are we can keep the existing Enum and code for converting the Enum to a string. This would also allow updating the function signatures with a scripted-diff, which makes larger refactors like this easier to verify as a reviewer.
>
> It also feels like a more natural fit to me and my gut feeling is that having a class to encapsulate the removal reasons and necessary data will be more maintainable and extensible going forward.
@josibake Makes sense. I'll create a RemovalR
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2108566561)
> The benefits I see are we can keep the existing Enum and code for converting the Enum to a string. This would also allow updating the function signatures with a scripted-diff, which makes larger refactors like this easier to verify as a reviewer.
>
> It also feels like a more natural fit to me and my gut feeling is that having a class to encapsulate the removal reasons and necessary data will be more maintainable and extensible going forward.
@josibake Makes sense. I'll create a RemovalR
...
💬 ryanofsky commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#discussion_r1598906084)
Whatever you prefer is good. I like getting rid of the nested scope, and the current PR is fine. I was just suggesting keeping the comment as a reminder that the import variable would be change to false at that point.
(https://github.com/bitcoin/bitcoin/pull/30083#discussion_r1598906084)
Whatever you prefer is good. I like getting rid of the nested scope, and the current PR is fine. I was just suggesting keeping the comment as a reminder that the import variable would be change to false at that point.
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2108576097)
There is another CI failure in https://cirrus-ci.com/task/6531594385620992:
```
crypto/.libs/libbitcoin_crypto_base.a(crypto_libbitcoin_crypto_base_la-cleanse.o): in function `memory_cleanse(void*, unsigned long)':
cleanse.cpp:(.text+0x0): multiple definition of `memory_cleanse(void*, unsigned long)'; /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x22
support/.libs/libbitcoinkernel_la-cleanse.o:cleanse.cpp:(.text+0x0): first defined here
clang: error: linker command failed with
...
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2108576097)
There is another CI failure in https://cirrus-ci.com/task/6531594385620992:
```
crypto/.libs/libbitcoin_crypto_base.a(crypto_libbitcoin_crypto_base_la-cleanse.o): in function `memory_cleanse(void*, unsigned long)':
cleanse.cpp:(.text+0x0): multiple definition of `memory_cleanse(void*, unsigned long)'; /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x22
support/.libs/libbitcoinkernel_la-cleanse.o:cleanse.cpp:(.text+0x0): first defined here
clang: error: linker command failed with
...
💬 jonatack commented on pull request "p2p: detect addnode cjdns peers in GetAddedNodeInfo()":
(https://github.com/bitcoin/bitcoin/pull/30085#issuecomment-2108593752)
This straightforward fix was previously ACKed by @vasild (https://github.com/bitcoin/bitcoin/pull/28248#pullrequestreview-1695032063 and Concept ACKed by @brunoerg (https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1777344970) with a request to add test coverage, which I've added here.
@vasild @brunoerg mind having a look (thanks!)
(https://github.com/bitcoin/bitcoin/pull/30085#issuecomment-2108593752)
This straightforward fix was previously ACKed by @vasild (https://github.com/bitcoin/bitcoin/pull/28248#pullrequestreview-1695032063 and Concept ACKed by @brunoerg (https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1777344970) with a request to add test coverage, which I've added here.
@vasild @brunoerg mind having a look (thanks!)
💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598938018)
hmm that's true, maybe I can quickly hack something together. It's probably better to compare the base index anyways.
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598938018)
hmm that's true, maybe I can quickly hack something together. It's probably better to compare the base index anyways.