💬 fanquake commented on issue "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0":
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2388086723)
> Tried on Intel macOS 15.0 (Xcode 16.0) and 13.7 (Xcode 15.2):

Tried which code? If this was a clean build, according to the log, it's also skipped building Boost for some reason?

This looks odd, because you seem to be hitting code paths for macOS cross-compilation, when building natively. i.e when building on macOS, the tooling should be found by calling `xcrun`:

https://github.com/bitcoin/bitcoin/blob/fc642c33ef28829eda0119a0fe39fd9bc4b84051/depends/builders/darwin.mk#L4-L8

I got
...

💬 fanquake commented on pull request "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0":
(https://github.com/bitcoin/bitcoin/pull/30994#issuecomment-2388089344)
Not sure about this issue, or making this change. Looks like it needs more investigation. See my comment here: https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2388086723.

💬 antonilol commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2388103756)
> According to https://en.wikipedia.org/wiki/SipHash key size is 128-bit as standard and that's what is currently supported in Bitcoin Core. Having a smaller key might increase DoS risk to much after all?
> + The key is only stored in one entry in LevelDB.
> + This type of computation using data that is cache-coherent (8 bytes next to the first 8 bytes of the key) should be exceedingly cheap.

You are right, siphash is designed for a 128 bit key, so lets keep it 128 bit then. It is not a cry
...

💬 fanquake commented on pull request "doc/build-osx.md:brew relinking note":
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1784172617)
> This PR isn't in response to any issue or comment. I just noticed it may be useful to have a note about relinking.

We don't really add general things to our build instructions that "may" be useful, especially if they aren't addressing a problem someone has encountered. In any case, in this instance, if your package manager is broken, or incorrectly installing dependencies such that they are broken, that isn't a Bitcoin Core issue, and I don't think we need to add anything here.

💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2388163491)
> They are unrelated to the checksum, so if they are failing, it seems like this patch is incomplete and some part is still checking the magic or checksum, no?

I'm guessing that we need to make sure that (if we are fuzzing) `V1Transport::SetMessageToSend` creates valid checksums/magic-bytes according to the new fuzzing mode check.

✅ itornaza closed a pull request: "refactor: Appropriate re-naming of MAX_OPCODE after tapscript"
(https://github.com/bitcoin/bitcoin/pull/30953)

💬 itornaza commented on pull request "refactor: Appropriate re-naming of MAX_OPCODE after tapscript":
(https://github.com/bitcoin/bitcoin/pull/30953#issuecomment-2388190304)
@darosior, highly respecting your opinion and taking note of your suggestions, I am close this pr.

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2388224233)
> Can you explain why the asserts and sanity checks would be failing? They are unrelated to the checksum, so if they are failing, it seems like this patch is incomplete and some part is still checking the magic or checksum, no?

Some previously valid magic or checksum could be now be considered invalid. But I removed the commit that bypasses the asserts and changed the verification to:
```
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
if ((hdr.pchMessageStart[0] & 1) != 0 && hdr.pchM
...

✅ fanquake closed an issue: "ci: Intermittent issue in feature_fee_estimation.py in sanity_check_rbf_estimates (Block sync timed out after 2400s)"
(https://github.com/bitcoin/bitcoin/issues/30990)

✅ fanquake closed an issue: "Intermittent failure in feature_fee_estimation.py in sanity_check_rbf_estimates: est_feerate = node.estimatesmartfee(2)["feerate"] (KeyError: 'feerate')"
(https://github.com/bitcoin/bitcoin/issues/30640)

🚀 fanquake merged a pull request: "test: add missing sync to feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31016)

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784260961)
I used `std::string` at first too, but then it was inconsistent with the rest of the documentation, which I think is even more unclear. I'll reconsider updating all `std::string` references in this block if I have to force-push again unless you think this is blocking?

💬 hebasto commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2388309124)
This discussion might be related: https://discourse.cmake.org/t/is-there-a-downside-for-a-toolchain-to-set-vars-in-cache-instead-of-init/12034.

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388310577)
> It would be cleaner _in the implementation_ to use `util::Result` instead of overwriting the returned value with the error message

I don't think that's a good approach for our codebase, even if there was minimal code churn. All format strings are known at compile-time, so any formatting errors are programming errors. Handling that with `util::Result` would be both a cumbersome interface and bad practice imo. We don't need to let callsites handle formatting errors, they should never occur, b
...

✅ maflcko closed an issue: "Intermittent timeout in tsan feature_init.py"
(https://github.com/bitcoin/bitcoin/issues/30586)

💬 maflcko commented on issue "Intermittent timeout in tsan feature_init.py":
(https://github.com/bitcoin/bitcoin/issues/30586#issuecomment-2388314370)
Closing for now, but this can be reopened when it happens again the next time.

🚀 fanquake merged a pull request: "doc: add testnet4 section header for config file"
(https://github.com/bitcoin/bitcoin/pull/31007)

💬 fanquake commented on pull request "wallet: Filter-out "send" addresses from `listreceivedby*`":
(https://github.com/bitcoin/bitcoin/pull/25973#issuecomment-2388319477)
Some of this was picked up in #30972.

💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388324332)
I agree that `util::Result` doesn't make any sense here. Apart from the reasons mentioned, it would also make it harder to switch to `std::format`, as well as being confusing, because no other format lib I am aware of is doing that.

> * using the `tfm::format_raw` functions (only for tinyformat implementation or tests)

In the PR description: I think this should also say `or bitcoin-cli`.

💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784285299)
If `Tinyformat handles string parameters.` is too controversial, I think it can be removed in the follow-up that removes the linter.

Otherwise, if this is replaced with `std::string`, someone else is going to suggest to mention `std::string_view` as well.