🤔 frankomosh reviewed a pull request: "fuzz: compact block harness"
(https://github.com/bitcoin/bitcoin/pull/33300#pullrequestreview-3586886869)
Concept ACK. fuzzing direction look reasonable.
(https://github.com/bitcoin/bitcoin/pull/33300#pullrequestreview-3586886869)
Concept ACK. fuzzing direction look reasonable.
💬 frankomosh commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2626463355)
Seems like `txns_randomized` reorders between runs, could this cause the same fuzz seed to pick different transactions? If so, would it make sense to snapshot the vector first for stable indexing?
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2626463355)
Seems like `txns_randomized` reorders between runs, could this cause the same fuzz seed to pick different transactions? If so, would it make sense to snapshot the vector first for stable indexing?
💬 frankomosh commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2626340174)
Since this helper appears to be used only by fuzz test code, is there any downside to keeping it in a test only location(i.e under `src/test/util`) instead of `src/util/fs.h`? Maybe that could help keep util’s interface focused on production use, but happy to defer if there’s a reason it needs to live here specifically.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2626340174)
Since this helper appears to be used only by fuzz test code, is there any downside to keeping it in a test only location(i.e under `src/test/util`) instead of `src/util/fs.h`? Maybe that could help keep util’s interface focused on production use, but happy to defer if there’s a reason it needs to live here specifically.
🤔 stickies-v reviewed a pull request: "log: Use `__func__` for -logsourcelocations"
(https://github.com/bitcoin/bitcoin/pull/34088#pullrequestreview-3587361950)
Concept ACK. I agree that for some functions (such as your example), the verbosity is a downside.
Have you considered just capping the `function_name()` to, say, 20 characters, when printing? (could even make it fixed length to make logs a bit more uniform for readability) I think that'd be about as helpful without having to introduce our own `SourceLocation`?
(https://github.com/bitcoin/bitcoin/pull/34088#pullrequestreview-3587361950)
Concept ACK. I agree that for some functions (such as your example), the verbosity is a downside.
Have you considered just capping the `function_name()` to, say, 20 characters, when printing? (could even make it fixed length to make logs a bit more uniform for readability) I think that'd be about as helpful without having to introduce our own `SourceLocation`?
💬 maflcko commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#issuecomment-3665069468)
> Have you considered just capping the `function_name()` to, say, 20 characters, when printing? (could even make it fixed length to make logs a bit more uniform for readability) I think that'd be about as helpful without having to introduce our own `SourceLocation`?
I don't think this works, because the return type is included in the function signature. I think printing something short but wrong is worse than printing the full thing.
Some examples:
```
std::variant<int, double, float,
...
(https://github.com/bitcoin/bitcoin/pull/34088#issuecomment-3665069468)
> Have you considered just capping the `function_name()` to, say, 20 characters, when printing? (could even make it fixed length to make logs a bit more uniform for readability) I think that'd be about as helpful without having to introduce our own `SourceLocation`?
I don't think this works, because the return type is included in the function signature. I think printing something short but wrong is worse than printing the full thing.
Some examples:
```
std::variant<int, double, float,
...
⚠️ rkrux opened an issue: "GUI crashes in regtest"
(https://github.com/bitcoin-core/gui/issues/918)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
The GUI crashes in regtest upon loading. It did work once initially but has been crashing since, tried quite a few times.
### Expected behaviour
The GUI should not crash in regtest, just like it doesn't in main or test environments.
### Steps to reproduce
Following is the command I have used. Also, tried a few variants of the `-datadir, -walletdir` args by adding and removing `/regtest
...
(https://github.com/bitcoin-core/gui/issues/918)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
The GUI crashes in regtest upon loading. It did work once initially but has been crashing since, tried quite a few times.
### Expected behaviour
The GUI should not crash in regtest, just like it doesn't in main or test environments.
### Steps to reproduce
Following is the command I have used. Also, tried a few variants of the `-datadir, -walletdir` args by adding and removing `/regtest
...
🤔 stickies-v reviewed a pull request: "scripted-diff: [doc] Unify stale copyright headers"
(https://github.com/bitcoin/bitcoin/pull/34084#pullrequestreview-3587528249)
Approach ACK. I've moved to a dateless copyright notice for new code, personally. The scripted-diff keeping the initial publication dates seems like the approach with the least friction.
(https://github.com/bitcoin/bitcoin/pull/34084#pullrequestreview-3587528249)
Approach ACK. I've moved to a dateless copyright notice for new code, personally. The scripted-diff keeping the initial publication dates seems like the approach with the least friction.
🤔 maflcko reviewed a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3587318193)
lgtm, but I haven't reviewed the ABC part and I wonder why it is needed.
review ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f 🍶
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU
...
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3587318193)
lgtm, but I haven't reviewed the ABC part and I wonder why it is needed.
review ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f 🍶
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU
...
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626777661)
nit: Could use `!bm.IsBlockPruned(*index)` for a stricter check?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626777661)
nit: Could use `!bm.IsBlockPruned(*index)` for a stricter check?
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626714452)
nit: Could use `IsValid(BLOCK_VALID_TREE)` for a stricter check? Also, to avoid a (silent) conflict with https://github.com/bitcoin/bitcoin/pull/32950 ?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626714452)
nit: Could use `IsValid(BLOCK_VALID_TREE)` for a stricter check? Also, to avoid a (silent) conflict with https://github.com/bitcoin/bitcoin/pull/32950 ?
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626894721)
nit: Could use the stricter `!bm.IsBlockPruned(prune_block)` check?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626894721)
nit: Could use the stricter `!bm.IsBlockPruned(prune_block)` check?
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626748203)
nit: Could use `IsValid(BLOCK_VALID_TRANSACTIONS)` for a stricter check?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626748203)
nit: Could use `IsValid(BLOCK_VALID_TRANSACTIONS)` for a stricter check?
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626858638)
q: Maybe I am missing something, but what is the benefit of re-implementing this. `ConnectTip` is already `protected`, so it may be possible to mock it to inject invalidity?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626858638)
q: Maybe I am missing something, but what is the benefit of re-implementing this. `ConnectTip` is already `protected`, so it may be possible to mock it to inject invalidity?
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626711907)
nit: Could use `IsValid(BLOCK_VALID_TREE)` for a stricter check?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626711907)
nit: Could use `IsValid(BLOCK_VALID_TREE)` for a stricter check?
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626902739)
same nit here for the asserts in the third commit.
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626902739)
same nit here for the asserts in the third commit.
👍 sedited approved a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3587573179)
Re-ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3587573179)
Re-ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f
💬 rkrux commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2626938085)
Updated, good catch!
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2626938085)
Updated, good catch!
📝 maflcko opened a pull request: "move-only: MAX_BLOCK_TIME_GAP to src/qt"
(https://github.com/bitcoin-core/gui/pull/919)
`MAX_BLOCK_TIME_GAP` was used in some incorrect heuristics, which were removed in commit e30b6ea194fee3bb95a45e7b732a99566b88f1f5.
This leaves a single module in src/qt using the constant.
Instead of exposing it in a central kernel header, just move it to the single gui module that uses it.
(https://github.com/bitcoin-core/gui/pull/919)
`MAX_BLOCK_TIME_GAP` was used in some incorrect heuristics, which were removed in commit e30b6ea194fee3bb95a45e7b732a99566b88f1f5.
This leaves a single module in src/qt using the constant.
Instead of exposing it in a central kernel header, just move it to the single gui module that uses it.
👍 rkrux approved a pull request: "scripted-diff: [doc] Unify stale copyright headers"
(https://github.com/bitcoin/bitcoin/pull/34084#pullrequestreview-3587704436)
ACK fa5f29774872d18febc0df38831a6e45f3de69cc
Previously, I had noticed that the copyright dates were updated in the files that were affected in the pull requests by few contributors. Fixing all in one go appears to be a good idea to me.
I built and ran the functional tests, all work.
The Github UI is crashing due to the size of the diff, thus I checked out the code locally and opened the commit diff in a file to count the number of occurrences of "The Bitcoin Core developers" phrase that match
...
(https://github.com/bitcoin/bitcoin/pull/34084#pullrequestreview-3587704436)
ACK fa5f29774872d18febc0df38831a6e45f3de69cc
Previously, I had noticed that the copyright dates were updated in the files that were affected in the pull requests by few contributors. Fixing all in one go appears to be a good idea to me.
I built and ran the functional tests, all work.
The Github UI is crashing due to the size of the diff, thus I checked out the code locally and opened the commit diff in a file to count the number of occurrences of "The Bitcoin Core developers" phrase that match
...
💬 fanquake commented on pull request "move-only: MAX_BLOCK_TIME_GAP to src/qt":
(https://github.com/bitcoin-core/gui/pull/919#issuecomment-3665316633)
cc @sedited
(https://github.com/bitcoin-core/gui/pull/919#issuecomment-3665316633)
cc @sedited