💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2363454663)
Fixed ci

📝 theStack opened a pull request: "scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937)
This PR is a follow-up to #30856, as suggested in comment https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356804690. With the scripted diff, review should be fairly trivial, but it could still be seen as controversial due to the large number of files (78 in total) being touched.

💬 ismaelsadeeq commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768427965)
nit
```suggestion
self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node")

```

💬 ismaelsadeeq commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768424398)
nit:
Assert that the chain height is 214, because the test assume so?
```suggestion
# Ensure the chain tip is at height 214, because this test assume it is.
assert_equal(node.getchaintips()[0]["height"], 214)
# We need a few more blocks so we can actually get above an realistic
```

🤔 ismaelsadeeq reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2318004044)
code review ACK 38648f4940eb13ebc858081c63e587156428107a

nice adding 38648f4940eb13ebc858081c63e587156428107a 👍🏾

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768447740)
While posting this PR I was aware of your `tuple_cat` suggestion. I am not against it on principle but TBH I haven't had a clear reason to struggle through that corner of C++ and don't see your suggestion compelling enough to work through it. Your suggestion in that comment is close to compiling but doesn't handle sending in one arg less for the failure cases. A motivation behind this PR is to prove parity in a clear way, and I'm worried it might obfuscate it. Happy to learn more from a diff.

...

💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2363533007)
The full tail of the log is below. It shows that 202 fuzz targets have been selected, but only 201 completed.

Not sure which one is missing, but it seems odd that one target would take more than 1.5 hours, when previously everything passed in less than .5 hours.

<details><summary>full log tail</summary>

```
2024-09-20T09:37:21.3001180Z ALL | ✓ Passed | 2778 s (accumulated)
2024-09-20T09:37:21.3001780Z [0mRuntime: 447 s
2024-09-2
...

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768457266)
Thanks, it is fine. I may take a look next week to recover the diff from when I wrote this earlier this month, but this minor style nit certainly isn't important at all.

🤔 hebasto reviewed a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2318027774)
My Guix build:
```
aarch64
f65f7839cb8ce4b194ab55a698b9840dd335d790a5499d3303cf79885405bedb guix-build-7025942687fd/output/aarch64-linux-gnu/SHA256SUMS.part
7953c080586b3500c73edd93731b8b9bcfd79ac4f5fe7953d51dc54643899255 guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu-debug.tar.gz
c82339ac1813727df996deda765087d430080a62d5fff7e16f46727be703a68c guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu.tar.gz
ffe2b02e
...

🤔 hebasto reviewed a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2318031732)
Post-merge re-ACK 7025942687fd5e91d0a10ce5b2ac673b67a63491.

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1768466480)
Sure, done!

💬 maflcko commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2363634967)
Thanks! However, I think there was some kind of issue with the fuzz inputs, so that the macOS 14 CI ran into timeouts.

I've removed them temporarily again in https://github.com/bitcoin-core/qa-assets/commit/84cea7068728bc2c765b9da680eedcb85f9f3c1b . This should allow for some time to investigate, while unbreaking the CI.

💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2363637034)
Sorry, wrong alarm. The timeout may be real. It may be `p2p_headers_presync` from https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2363634967

💬 maflcko commented on pull request "ci: Approximate MAKEJOBS in image build phase":
(https://github.com/bitcoin/bitcoin/pull/30935#issuecomment-2363643874)
(This is a follow-up to https://github.com/bitcoin/bitcoin/pull/28504)

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1768582575)
I renamed it to `try_format` to be clear about the different behaviour compared to `tfm::format`, but agreed that avoiding merge conflicts is useful to coordinate on.

> My preference would be to leave the name as-is and instead just delete the unchecked one

Could you elaborate on this a bit more? We have these 4 other `format` overloads, I don't think you want to just remove them all? At least 2) and 4) would imply touching quite a bit of code to refactor, and it seems not all call sites o
...

💬 maflcko commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1768605114)
IIUC the compiler will lookup `format(const char*, ...)` for `format("%s, 1)`, so `format(const char*, ...)` would have to be deleted for the compiler to pick `format(ConstevalFormatString, ...)`. (My recommendation for this pull would be to do at least this)

IIUC the remaining `format` taking a `std::string` can be left as-is for now, as they are less common anyway than the ones taking a compile-time string literal. (Possibly something can be done for them as well, but it seems fine to leave
...

🤔 pablomartin4btc reviewed a pull request: "refactor: move `SignSignature` helpers to test utils"
(https://github.com/bitcoin/bitcoin/pull/30561#pullrequestreview-2318311398)
ACK 58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070

The helpers are already used exclusively within the test code; this is a suitable cleanup.

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1768664595)
> so `format(const char*, ...)` would have to be deleted for the compiler to pick `format(ConstevalFormatString, ...)`

I think just deleting `format(const char*, ...)` leads to ambiguous overloading?

```
../../../../src/checkqueue.h:136:36: error: call to 'format' is ambiguous
util::ThreadRename(strprintf("scriptch.%i", n));
^~~~~~~~~
../../../../src/tinyformat.h:1151:19: note: expanded from macro 'strprintf'
#define strprintf tfm::fo
...

💬 maflcko commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1768680022)
> I think just deleting `format(const char*, ...)` leads to ambiguous overloading?

Thanks for actually checking. I guess this means that the `std::string` overload would have to be renamed to avoid the overload problem. Hopefully only the fuzz tests and a few lines in real code use the string overload?



> you also asked to remove `./test/lint/lint-format-strings.py` in this pull which I think we shouldn't do before all `tfm::format` overloads are compile-time checked? So that's why I'm
...

💬 marcofleon commented on pull request "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work":
(https://github.com/bitcoin/bitcoin/pull/30918#discussion_r1768704392)
That's true, good note.