🤔 vasild reviewed a pull request: "test: Do not write Python bytecode to source directory"
(https://github.com/bitcoin/bitcoin/pull/30533#pullrequestreview-2242364341)
ACK c91a10b6843b9c48cb200592e025bd2a0fcb8a7e in case this is reconsidered.
IMO this approach is better than leaving it as it is.
(https://github.com/bitcoin/bitcoin/pull/30533#pullrequestreview-2242364341)
ACK c91a10b6843b9c48cb200592e025bd2a0fcb8a7e in case this is reconsidered.
IMO this approach is better than leaving it as it is.
👍 danielabrozzoni approved a pull request: "contrib/signet/miner updates"
(https://github.com/bitcoin/bitcoin/pull/28417#pullrequestreview-2242377679)
Code review ACK fb6d51eb25a2bb69a3ecdfc4796a88d4d1aacc65
(https://github.com/bitcoin/bitcoin/pull/28417#pullrequestreview-2242377679)
Code review ACK fb6d51eb25a2bb69a3ecdfc4796a88d4d1aacc65
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1719646767)
CI is failing because it halts at 128 iters, it seems.
Original iters given was 2^(16/2-1) = 128.
However, sqrt(2^(n-1)) = sqrt(2^15) > 150
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1719646767)
CI is failing because it halts at 128 iters, it seems.
Original iters given was 2^(16/2-1) = 128.
However, sqrt(2^(n-1)) = sqrt(2^15) > 150
🤔 fjahr reviewed a pull request: "seeds: Pull additional nodes from my seeder and update fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/30008#pullrequestreview-2242371066)
tACK 7f55140007186cda876ad0a5da812e391cddbcc4
I reviewed the code and the changes look correct to me. I tested that the updated instructions in the README work as expected and they did (though see my comment on the testnet4 line). The resulting files showed some reasonable differences the files included here, which is expected. I also confirmed that generating `chainparamsseeds.h` from the txt files included here yields the same result.
(https://github.com/bitcoin/bitcoin/pull/30008#pullrequestreview-2242371066)
tACK 7f55140007186cda876ad0a5da812e391cddbcc4
I reviewed the code and the changes look correct to me. I tested that the updated instructions in the README work as expected and they did (though see my comment on the testnet4 line). The resulting files showed some reasonable differences the files included here, which is expected. I also confirmed that generating `chainparamsseeds.h` from the txt files included here yields the same result.
💬 fjahr commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719636813)
nit: Typo in the commit message of 6c2a87fb1ccf26daa754e6d5a4c4ae687da562e3: "tesnet4"
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719636813)
nit: Typo in the commit message of 6c2a87fb1ccf26daa754e6d5a4c4ae687da562e3: "tesnet4"
💬 fjahr commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719648490)
Hm, either I'm missing something or this would mean that the testnet3 nodes would still be included here? Since you say this isn't really working yet (how I understand this [here](https://github.com/bitcoin/bitcoin/pull/30008#issuecomment-2289394756)) maybe rather put a TODO here instead?
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719648490)
Hm, either I'm missing something or this would mean that the testnet3 nodes would still be included here? Since you say this isn't really working yet (how I understand this [here](https://github.com/bitcoin/bitcoin/pull/30008#issuecomment-2289394756)) maybe rather put a TODO here instead?
💬 fjahr commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719652387)
How up-to-date is that ASMap file? If it's not up updated regularly it might be better to use one of the ones available at https://github.com/fjahr/asmap-data. Maybe this is a change in process that requires more eyes on it and I wouldn't call this a blocker then but I would like to propose this as a goal for the next release to use something more recent.
cc: @sipa
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719652387)
How up-to-date is that ASMap file? If it's not up updated regularly it might be better to use one of the ones available at https://github.com/fjahr/asmap-data. Maybe this is a change in process that requires more eyes on it and I wouldn't call this a blocker then but I would like to propose this as a goal for the next release to use something more recent.
cc: @sipa
💬 fjahr commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719633747)
nit: If we want this `MIN_BLOCKS` to default to mainnet then you could update it to `840_000` while you make edits here. But maybe it would be better to have a more concise behavior here in the future. I guess the file could have a header with the network, number of min blocks that makes sense etc. based on the numbers the seeder node has.
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719633747)
nit: If we want this `MIN_BLOCKS` to default to mainnet then you could update it to `840_000` while you make edits here. But maybe it would be better to have a more concise behavior here in the future. I guess the file could have a header with the network, number of min blocks that makes sense etc. based on the numbers the seeder node has.
💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293291058)


The second one just shows the same plot zoomed in on the last 600k iterations or so. I'm going to follow up with a plot of executions vs time as well. And I'll do more than a million iterations, as it seems like `utxo_snapshot_invalid` might end up being faster if fuzzed for hours.
I compiled using only `
...
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293291058)


The second one just shows the same plot zoomed in on the last 600k iterations or so. I'm going to follow up with a plot of executions vs time as well. And I'll do more than a million iterations, as it seems like `utxo_snapshot_invalid` might end up being faster if fuzzed for hours.
I compiled using only `
...
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293319752)
@marcofleon Thanks for checking. I presume you checked the `utxo_snapshot` target, not the `utxo_snapshot_invalid` target and you found that the new target is slower in your run? I guess it is a bit hard to compare a random search path done by the fuzz engine in the input search space, because different paths may take a different time. As mentioned earlier, improving the "valid" target isn't a goal of this pull (https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2291393187). However, I'd
...
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293319752)
@marcofleon Thanks for checking. I presume you checked the `utxo_snapshot` target, not the `utxo_snapshot_invalid` target and you found that the new target is slower in your run? I guess it is a bit hard to compare a random search path done by the fuzz engine in the input search space, because different paths may take a different time. As mentioned earlier, improving the "valid" target isn't a goal of this pull (https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2291393187). However, I'd
...
💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293339170)
Thank you for the suggestions, I'll do more trials for sure. That actually is the `utxo_snapshot_invalid` target compared to the old `utxo_snapshot` (sorry, file names should've been clearer). It's much faster at the beginning. I ran it from scratch, not on any corpus. I'm generating a corpus now for the invalid target and then I'll do a comparison for both when fuzzing from a corpus.
Also, for the bug from https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285605841. What's the r
...
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293339170)
Thank you for the suggestions, I'll do more trials for sure. That actually is the `utxo_snapshot_invalid` target compared to the old `utxo_snapshot` (sorry, file names should've been clearer). It's much faster at the beginning. I ran it from scratch, not on any corpus. I'm generating a corpus now for the invalid target and then I'll do a comparison for both when fuzzing from a corpus.
Also, for the bug from https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285605841. What's the r
...
💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293345228)
To clarify, I reproduced the bug fairly quickly with the line you provided in https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2290858777. I would like to see what happens with `max_len` not specified.
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293345228)
To clarify, I reproduced the bug fairly quickly with the line you provided in https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2290858777. I would like to see what happens with `max_len` not specified.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293350198)
> Thank you for the suggestions, I'll do more trials for sure. That actually is the `utxo_snapshot_invalid` target compared to the old `utxo_snapshot` (sorry, file names should've been clearer).
Are you sure you are on the right commit binaries? Locally I am seeing a speed difference of night and day. 1e6 iterations with the new target should happen within a few minutes, whereas with the old it would take hours/days.
> Also, for the bug from https://github.com/bitcoin/bitcoin/issues/30514#
...
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293350198)
> Thank you for the suggestions, I'll do more trials for sure. That actually is the `utxo_snapshot_invalid` target compared to the old `utxo_snapshot` (sorry, file names should've been clearer).
Are you sure you are on the right commit binaries? Locally I am seeing a speed difference of night and day. 1e6 iterations with the new target should happen within a few minutes, whereas with the old it would take hours/days.
> Also, for the bug from https://github.com/bitcoin/bitcoin/issues/30514#
...
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293358037)
I just re-tried `FUZZ=utxo_snapshot_invalid ./fuzz_exe_fa0b66400c12d11579b38b1ac76f677b23e8c82a -seed=1337 -runs=1000000` and it took less time than to write the second-last comment, and as much time as `FUZZ=utxo_snapshot ./fuzz_exe_389cf32aca0be6c4b01151c92cc3be79c120f197 -seed=1337 -runs=50000`.
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293358037)
I just re-tried `FUZZ=utxo_snapshot_invalid ./fuzz_exe_fa0b66400c12d11579b38b1ac76f677b23e8c82a -seed=1337 -runs=1000000` and it took less time than to write the second-last comment, and as much time as `FUZZ=utxo_snapshot ./fuzz_exe_389cf32aca0be6c4b01151c92cc3be79c120f197 -seed=1337 -runs=50000`.
💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293359132)
I'll recheck my setup and get back to you.
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293359132)
I'll recheck my setup and get back to you.
💬 fjahr commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719736632)
I raise you 17G! (height 2873788)
```
$ du -sh /Volumes/PortableSSD/datadir/testnet3/chainstate/ /Volumes/PortableSSD/datadir/testnet3/blocks/
17G /Volumes/PortableSSD/datadir/testnet3/chainstate/
82G /Volumes/PortableSSD/datadir/testnet3/blocks/
```
I know LevelDB can cause some differences in size on disk but this seems extreme...
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719736632)
I raise you 17G! (height 2873788)
```
$ du -sh /Volumes/PortableSSD/datadir/testnet3/chainstate/ /Volumes/PortableSSD/datadir/testnet3/blocks/
17G /Volumes/PortableSSD/datadir/testnet3/chainstate/
82G /Volumes/PortableSSD/datadir/testnet3/blocks/
```
I know LevelDB can cause some differences in size on disk but this seems extreme...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719742146)
Yes, #29369 looks like a possible fix. Applied @ryanofsky's workaround for now but will have a look at that PR.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719742146)
Yes, #29369 looks like a possible fix. Applied @ryanofsky's workaround for now but will have a look at that PR.
💬 fjahr commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719750035)
Either way, `m_assumed_blockchain_size` can be updated here too.
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719750035)
Either way, `m_assumed_blockchain_size` can be updated here too.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719753108)
Ah, hadn't seen your #25227 before. It seems easier to default to `std::byte` when introducing `TryParseHex` without any prior callers. But thanks to your elaboration on the effort already under way to use `std::byte`, I did some more grepping and decided it might be worth attempting. Changed defaults in latest push 01e18d94d9577f415748869376988e3f0f59ced0 and worked through the fallout. The scripted-diff ended up with a lot less work in that version.
(The push right before it in 3a96b9f4b56c
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719753108)
Ah, hadn't seen your #25227 before. It seems easier to default to `std::byte` when introducing `TryParseHex` without any prior callers. But thanks to your elaboration on the effort already under way to use `std::byte`, I did some more grepping and decided it might be worth attempting. Changed defaults in latest push 01e18d94d9577f415748869376988e3f0f59ced0 and worked through the fallout. The scripted-diff ended up with a lot less work in that version.
(The push right before it in 3a96b9f4b56c
...
💬 RandyMcMillan commented on pull request "Hide peers details":
(https://github.com/bitcoin-core/gui/pull/505#issuecomment-2293390861)
thanks!
(https://github.com/bitcoin-core/gui/pull/505#issuecomment-2293390861)
thanks!