💬 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.
💬 paplorinc commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2108651938)
ACK 57a06646952fed98c1c281f02fe58a0758a8ed5a
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2108651938)
ACK 57a06646952fed98c1c281f02fe58a0758a8ed5a
💬 ryanofsky commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108662179)
> > the stratum code would call a more well-defined interface
>
> I would like that too. Just not sure how to go about it.
I think in #29432, you could add a `src/interfaces/mining_service.h` header similar to [`src/interfaces/chain.h`](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/chain.h) that looks something like:
```c++
class MiningService {
public:
virtual ~MiningService() {}
// Returns a new block template based on the current state of the mempool and
...
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108662179)
> > the stratum code would call a more well-defined interface
>
> I would like that too. Just not sure how to go about it.
I think in #29432, you could add a `src/interfaces/mining_service.h` header similar to [`src/interfaces/chain.h`](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/chain.h) that looks something like:
```c++
class MiningService {
public:
virtual ~MiningService() {}
// Returns a new block template based on the current state of the mempool and
...
💬 sipa commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599001626)
This is not DER encoding. DER is used for signatures. It's defined in section 2.3.3 of SEC1 v1.9 (https://www.secg.org/sec1-v2.pdf), though I believe ANSI and/or NIST also standardized it.
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599001626)
This is not DER encoding. DER is used for signatures. It's defined in section 2.3.3 of SEC1 v1.9 (https://www.secg.org/sec1-v2.pdf), though I believe ANSI and/or NIST also standardized it.
💬 sipa commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599015256)
If you want, all the EC logic for point decompression is in `test/functional/test_framework/crypto/secp256k1.py1`:
```python
from test_framework.crypto.secp256k1 import G, FE, GE
from hashlib import sha256
print(sha256(G.to_bytes_uncompressed()).digest().hex())
```
(and to test it's actually an X coordinate):
```python
assert GE.lift_x(FE.from_bytes(sha256(G.to_bytes_uncompressed()).digest())) is not None
```
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599015256)
If you want, all the EC logic for point decompression is in `test/functional/test_framework/crypto/secp256k1.py1`:
```python
from test_framework.crypto.secp256k1 import G, FE, GE
from hashlib import sha256
print(sha256(G.to_bytes_uncompressed()).digest().hex())
```
(and to test it's actually an X coordinate):
```python
assert GE.lift_x(FE.from_bytes(sha256(G.to_bytes_uncompressed()).digest())) is not None
```
💬 achow101 commented on pull request "validation: don't clear cache on periodic flush: >2x block connection speed":
(https://github.com/bitcoin/bitcoin/pull/28233#issuecomment-2108699844)
ACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b
(https://github.com/bitcoin/bitcoin/pull/28233#issuecomment-2108699844)
ACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b
💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1599027343)
Didn't look deeply, but with the following diff this branch builds again after rebase to current master ff8c606cf1eaefd0eab9f144561120a and the unit tests pass:
```diff
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -45,7 +45,6 @@
#include <test/util/txmempool.h>
-#include <timedata.h>
#include <txdb.h>
@@ -648,10 +647,6 @@ NetTestingSetup::~NetTestingSetup()
fNameLookup = DEFAULT_NAME_LOOKUP;
fListen = true;
CreateSock = CreateSockOS;
...
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1599027343)
Didn't look deeply, but with the following diff this branch builds again after rebase to current master ff8c606cf1eaefd0eab9f144561120a and the unit tests pass:
```diff
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -45,7 +45,6 @@
#include <test/util/txmempool.h>
-#include <timedata.h>
#include <txdb.h>
@@ -648,10 +647,6 @@ NetTestingSetup::~NetTestingSetup()
fNameLookup = DEFAULT_NAME_LOOKUP;
fListen = true;
CreateSock = CreateSockOS;
...
💬 ryanofsky 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-2108712060)
> Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, `std::variant<CTransactionRef, BlockData>`, where block data holds the block hash and block number.
The drawback of a separate data field is that there is no longer a guarantee that data accompanying the reason is always present when the reason is set, and always absent when it is not set. So it is possible for code to only partially initialize the RemovalRea
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2108712060)
> Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, `std::variant<CTransactionRef, BlockData>`, where block data holds the block hash and block number.
The drawback of a separate data field is that there is no longer a guarantee that data accompanying the reason is always present when the reason is set, and always absent when it is not set. So it is possible for code to only partially initialize the RemovalRea
...
💬 paplorinc commented on pull request "refactor: reserve memory allocation for transaction outputs":
(https://github.com/bitcoin/bitcoin/pull/30093#discussion_r1599029438)
Hey @theuni, excellent review, thanks!
1) yes, it's always empty, leaving it out is indeed more readable - and if people modify it in the future, they should just pay attention to this
2) nice, haven't noticed it, added the +1 (added you as co-author)
3) To be consistent with `txNew.vin`, I've changed it to `emplace_back` instead, the performance should be comparable - do you agree?
4) wanted to be focused only on the issue discovered during code review, but this is much better, thanks for t
...
(https://github.com/bitcoin/bitcoin/pull/30093#discussion_r1599029438)
Hey @theuni, excellent review, thanks!
1) yes, it's always empty, leaving it out is indeed more readable - and if people modify it in the future, they should just pay attention to this
2) nice, haven't noticed it, added the +1 (added you as co-author)
3) To be consistent with `txNew.vin`, I've changed it to `emplace_back` instead, the performance should be comparable - do you agree?
4) wanted to be focused only on the issue discovered during code review, but this is much better, thanks for t
...
✅ ryanofsky closed an issue: "Slow memory leak in v22.0?"
(https://github.com/bitcoin/bitcoin/issues/24542)
(https://github.com/bitcoin/bitcoin/issues/24542)