💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1447261373)
I prefer having a single function (as opposed to keeping the logic at the call site) for the following reasons:

* We avoid duplicating code at the RPC call sites:
* The logic for validating SFFO inputs would need to be duplicated at `send`, `fundrawtx` and `walletcreatefundedpsbt`. This is the code that checks that the ints passed are not duplicated and in bounds and previously used to be inside `rpc/FundTransaction`
* We have one place to validate all SFFO inputs:
* Currently, `sendm
...

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884688686)
> The CI failure here is a bit odd, because it only happens for ZeroMQ, and only when using what looks like clang-14?

I think the solution to this is just to keep our current Clang version requirement for macOS cross-compilation. Currently, we vendor our own Clang (17) and use 17 in Guix. Switching to using system Clang doesn't mean we now need to support clang-13 + when building for macOS, because we never did previously, and no-one building Core could have been using a version other than t
...

💬 theStack commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#issuecomment-1884782328)
Concept ACK

Will review in a bit. As a follow-up idea, maybe we could put the AssumeUTXO chain and snapshot creation in a shared module, in order to deduplicate code between feature_assumeutxo.py and wallet_assumeutxo.py? If we ever wanted to change the snapshot chainstate for e.g. more advanced tests, it'd be quite annoying having to touch several files.

💬 brunoerg commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1884820312)
Concept ACK

💬 TheCharlatan commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447366749)
Should be `-17`?

💬 TheCharlatan commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447369323)
Would be good to fix this here imo.

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447383682)
Thanks, fixed up.

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884850027)
Given the above, we can also avoid having to repatch Qt, to work around bugs there. Dropped that commit.

💬 sipa commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1884850577)
utACK d536e5a6325d1885224f36debdcc5245b94efe8a

💬 maflcko commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447386320)
This will break in a few months, when `clang` will become an alias for `clang-18`

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447388044)
Sure. I guess at that point we'll have to change it. Not sure what else to do given this is what Ubuntu ships. I assume we'll have to make other changes at the same time, if the CI is changing compilers etc out from under us?

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447395720)
I guess we can also just explictly install clang-17 and llvm-17 here, and then not have to worry about things, until those packages no-longer exist.

💬 TheCharlatan commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447417248)
More changes as in being able to handle setting `CC` to e.g. `clang-17` for darwin depends? That seems worthwhile, no?

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447421755)
> More changes as in being able to handle setting CC to e.g. clang-17 for darwin depends?

That should already work ok, I mean, hardcoding things like `command -v clang-17` in depends. If the preference is to install the *-17 toolchain, and pass `CC` and `CXX` explicitly, we can do that.

💬 fanquake commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447422466)
I've pushed up a commit to do so.

💬 maflcko commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447426271)
> Sure. I guess at that point we'll have to change it.

Well, if it is going to be changed, it may be better to do the change now. Otherwise it just seems like extra steps to reach the same point eventually?

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447427155)
> Well, if it is going to be changed, it may be better to do the change now.

Change it to what though?

💬 maflcko commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447430952)
> Change it to what though?

`apt install clang-17`, like in the tidy task, it is also possible to export a variable to set `CROSS_LLVM_V="17"`

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447441959)
Ok, done as I suggested above.

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884928086)
Moved to explicitly installing `*-17`. Added the missing `darwin_STRIP=llvm-strip` .