💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2262288560)
Yeah, I also could not find such documentation when I first wrote the patch, and ended up making a simple c program to just call `getfsstat` on my mounted exfat disk, to get the info out to use here. I am not at home now though so can't even pass said program to you right now.
> I'm wondering if it's always written like this or if it's sometimes exFAT (like you refer to it) or [ExFAT](https://support.apple.com/guide/disk-utility/file-system-formats-dsku19ed921c/mac) (like the apple doc does s
...
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2262288560)
Yeah, I also could not find such documentation when I first wrote the patch, and ended up making a simple c program to just call `getfsstat` on my mounted exfat disk, to get the info out to use here. I am not at home now though so can't even pass said program to you right now.
> I'm wondering if it's always written like this or if it's sometimes exFAT (like you refer to it) or [ExFAT](https://support.apple.com/guide/disk-utility/file-system-formats-dsku19ed921c/mac) (like the apple doc does s
...
💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2262292424)
Great, thanks Fedor!
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2262292424)
Great, thanks Fedor!
💬 mprenditore commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-3167154292)
Hello, anything still pending or can now be merged?
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-3167154292)
Hello, anything still pending or can now be merged?
👍 maflcko approved a pull request: "test: use local `CBlockIndex` in block read hash mismatch check"
(https://github.com/bitcoin/bitcoin/pull/33154#pullrequestreview-3100070124)
lgtm
(https://github.com/bitcoin/bitcoin/pull/33154#pullrequestreview-3100070124)
lgtm
💬 maflcko commented on pull request "test: use local `CBlockIndex` in block read hash mismatch check":
(https://github.com/bitcoin/bitcoin/pull/33154#discussion_r2262406431)
```suggestion
index.nDataPos = m_node.chainman->ActiveTip()->nDataPos;
```
nit: Would be nice to document explicitly what block this reads and also use the `m_node.chainman->GetMutex()` for new code.
(https://github.com/bitcoin/bitcoin/pull/33154#discussion_r2262406431)
```suggestion
index.nDataPos = m_node.chainman->ActiveTip()->nDataPos;
```
nit: Would be nice to document explicitly what block this reads and also use the `m_node.chainman->GetMutex()` for new code.
💬 maflcko commented on pull request "test: use local `CBlockIndex` in block read hash mismatch check":
(https://github.com/bitcoin/bitcoin/pull/33154#discussion_r2262406841)
```suggestion
```
nit: Can be removed?
(https://github.com/bitcoin/bitcoin/pull/33154#discussion_r2262406841)
```suggestion
```
nit: Can be removed?
👍 dergoegge approved a pull request: "refactor: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/33116#pullrequestreview-3100090085)
Code review ACK a20724d926d5844168c6a13fa8293df8c8927efe
(https://github.com/bitcoin/bitcoin/pull/33116#pullrequestreview-3100090085)
Code review ACK a20724d926d5844168c6a13fa8293df8c8927efe
📝 fanquake opened a pull request: "contrib: drop `bitcoin-util` exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/33155)
It's got `memcpy_chk`.
Guix Build (aarch64):
```bash
955c15e4b40dae5f0a80185a5a759764eb650003b56a997f2f18b058f979e212 guix-build-4bff4ce561b0/output/aarch64-linux-gnu/SHA256SUMS.part
4e57193921b5a96b1da82e964577e20fdc8f287de9f28dc15b818f94bdc12fd2 guix-build-4bff4ce561b0/output/aarch64-linux-gnu/bitcoin-4bff4ce561b0-aarch64-linux-gnu-debug.tar.gz
fb9779bfe3412f80393b85c7c12763fe34909879a34edbfe08d00bbea222d357 guix-build-4bff4ce561b0/output/aarch64-linux-gnu/bitcoin-4bff4ce561b0-aarch6
...
(https://github.com/bitcoin/bitcoin/pull/33155)
It's got `memcpy_chk`.
Guix Build (aarch64):
```bash
955c15e4b40dae5f0a80185a5a759764eb650003b56a997f2f18b058f979e212 guix-build-4bff4ce561b0/output/aarch64-linux-gnu/SHA256SUMS.part
4e57193921b5a96b1da82e964577e20fdc8f287de9f28dc15b818f94bdc12fd2 guix-build-4bff4ce561b0/output/aarch64-linux-gnu/bitcoin-4bff4ce561b0-aarch64-linux-gnu-debug.tar.gz
fb9779bfe3412f80393b85c7c12763fe34909879a34edbfe08d00bbea222d357 guix-build-4bff4ce561b0/output/aarch64-linux-gnu/bitcoin-4bff4ce561b0-aarch6
...
💬 fanquake commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2262433479)
This can just be removed: #33155.
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2262433479)
This can just be removed: #33155.
👍 dergoegge approved a pull request: "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts"
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3100133201)
Code review ACK 83950275eddacac56c58a7a3648ed435a5593328
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3100133201)
Code review ACK 83950275eddacac56c58a7a3648ed435a5593328
💬 hebasto commented on pull request "contrib: drop `bitcoin-util` exception from FORTIFY check":
(https://github.com/bitcoin/bitcoin/pull/33155#issuecomment-3167229653)
Concept ACK.
> It's got `memcpy_chk`.
It would be good to know when this happened.
(https://github.com/bitcoin/bitcoin/pull/33155#issuecomment-3167229653)
Concept ACK.
> It's got `memcpy_chk`.
It would be good to know when this happened.
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2262551801)
Just checking in to say it would be possible to add (and test) after https://github.com/bitcoin/bitcoin/pull/32588 by using `test_only_CheckFailuresAreExceptionsNotAborts` in the unit tests.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2262551801)
Just checking in to say it would be possible to add (and test) after https://github.com/bitcoin/bitcoin/pull/32588 by using `test_only_CheckFailuresAreExceptionsNotAborts` in the unit tests.
🤔 hebasto reviewed a pull request: "contrib: drop `bitcoin-util` exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/33155#pullrequestreview-3100258674)
The change looks good. Going to build on different platforms before ACKing.
(https://github.com/bitcoin/bitcoin/pull/33155#pullrequestreview-3100258674)
The change looks good. Going to build on different platforms before ACKing.
👍 maflcko approved a pull request: "log: rate limiting followups"
(https://github.com/bitcoin/bitcoin/pull/33011#pullrequestreview-3097038450)
nothing blocking, just very minor nits/questions.
review ACK 7bc28668cc791d28fe824476d2c1f236c5a772d0 🕙
<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+krxU1A3Yux4bpwZNLvVBKy0wLgM
...
(https://github.com/bitcoin/bitcoin/pull/33011#pullrequestreview-3097038450)
nothing blocking, just very minor nits/questions.
review ACK 7bc28668cc791d28fe824476d2c1f236c5a772d0 🕙
<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+krxU1A3Yux4bpwZNLvVBKy0wLgM
...
💬 maflcko commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2260242402)
nit in 0716f0a69f37d611ead459eadf35a8c670e511f4 (doesn't matter much): Generally, when a function exists in a class, it is better to use the `m_` prefix. This would also make the diff smaller.
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2260242402)
nit in 0716f0a69f37d611ead459eadf35a8c670e511f4 (doesn't matter much): Generally, when a function exists in a class, it is better to use the `m_` prefix. This would also make the diff smaller.
💬 maflcko commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2261098695)
f8b218418e8e716e210ee247e143172f6ef4fc93: why is this added?
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2261098695)
f8b218418e8e716e210ee247e143172f6ef4fc93: why is this added?
💬 maflcko commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2262558914)
nit in eb333104efd91b5cd994b1937cd4f81b8dd8c851: Why does truncation work, when this isn't flushed (ofs remains in scope), and even if it was flushed, is it guaranteed to be picked up by the other open file descriptor, which doesn't flush either after writes?
Would it be better to ftell and then read from there?
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2262558914)
nit in eb333104efd91b5cd994b1937cd4f81b8dd8c851: Why does truncation work, when this isn't flushed (ofs remains in scope), and even if it was flushed, is it guaranteed to be picked up by the other open file descriptor, which doesn't flush either after writes?
Would it be better to ftell and then read from there?
💬 maflcko commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2261066623)
nit in 65c8072757e58f9cad1198ddd8e403d656bb68e2: Unused/stray hunk?
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2261066623)
nit in 65c8072757e58f9cad1198ddd8e403d656bb68e2: Unused/stray hunk?
💬 maflcko commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2262504745)
should be in the next commit?
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2262504745)
should be in the next commit?
🤔 0xB10C reviewed a pull request: "CI: silent merge check"
(https://github.com/bitcoin/bitcoin/pull/33145#pullrequestreview-3100319037)
Have you considered doing something like the following in `.github/workflows/ci.yml` to avoid having to maintain separate files?
```
name: CI
on:
push:
pull_request:
types: [opened, reopened, synchronize, labeled]
jobs:
conditional-job:
if: >
github.event_name == 'push' ||
(
github.event_name == 'pull_request' &&
(
github.event.action == 'opened' ||
github.event.action == 'reopened' ||
github.event.action == 'synchronize'
...
(https://github.com/bitcoin/bitcoin/pull/33145#pullrequestreview-3100319037)
Have you considered doing something like the following in `.github/workflows/ci.yml` to avoid having to maintain separate files?
```
name: CI
on:
push:
pull_request:
types: [opened, reopened, synchronize, labeled]
jobs:
conditional-job:
if: >
github.event_name == 'push' ||
(
github.event_name == 'pull_request' &&
(
github.event.action == 'opened' ||
github.event.action == 'reopened' ||
github.event.action == 'synchronize'
...