💬 sipa commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1884850577)
utACK d536e5a6325d1885224f36debdcc5245b94efe8a
(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`
(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?
(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.
(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?
(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.
(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.
(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?
(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?
(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"`
(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.
(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` .
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884928086)
Moved to explicitly installing `*-17`. Added the missing `darwin_STRIP=llvm-strip` .
💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447466499)
> Do you mean incentive incompatible here? because in the given example if C is rejected (A,B) will be added to the mempool with mining score of 5s/vb which is lower than whats intended.
Comment
err right
src/rpc/client.cpp
Comment on lines +130 to +131
{ "submitpackage", 1, "maxfeerate" },
{ "submitpackage", 2, "maxburnamount" },
Member
@ismaelsadeeq ismaelsadeeq 2 days ago •
Should we just enable passing maxfeerate and maxburnamount as a config variables since it is used
...
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447466499)
> Do you mean incentive incompatible here? because in the given example if C is rejected (A,B) will be added to the mempool with mining score of 5s/vb which is lower than whats intended.
Comment
err right
src/rpc/client.cpp
Comment on lines +130 to +131
{ "submitpackage", 1, "maxfeerate" },
{ "submitpackage", 2, "maxburnamount" },
Member
@ismaelsadeeq ismaelsadeeq 2 days ago •
Should we just enable passing maxfeerate and maxburnamount as a config variables since it is used
...
💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447466859)
> Do you mean incentive incompatible here? because in the given example if C is rejected (A,B) will be added to the mempool with mining score of 5s/vb which is lower than whats intended.
correct, typo
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447466859)
> Do you mean incentive incompatible here? because in the given example if C is rejected (A,B) will be added to the mempool with mining score of 5s/vb which is lower than whats intended.
correct, typo
📝 maflcko opened a pull request: "ci: Rename tasks (previous releases, macOS cross)"
(https://github.com/bitcoin/bitcoin/pull/29218)
The previous releases task no longer uses the qt5 dev package, but the depends package, so fix that in the name.
Also, remove a detail from the macOS cross task name, because anyone can look it up in the source, if they really want to. Otherwise, it may go out of date in the name.
Also, rename the two tasks' config file to reflect the same.
(https://github.com/bitcoin/bitcoin/pull/29218)
The previous releases task no longer uses the qt5 dev package, but the depends package, so fix that in the name.
Also, remove a detail from the macOS cross task name, because anyone can look it up in the source, if they really want to. Otherwise, it may go out of date in the name.
Also, rename the two tasks' config file to reflect the same.
💬 jamesob commented on pull request "test: assumeutxo: spend coin from snapshot chainstate after loading":
(https://github.com/bitcoin/bitcoin/pull/29215#issuecomment-1884973094)
ACK https://github.com/bitcoin/bitcoin/pull/29215/commits/931575418e082af37b88b1819125b0d0f0fabbd5
Tested locally. Thanks, stackman!
(https://github.com/bitcoin/bitcoin/pull/29215#issuecomment-1884973094)
ACK https://github.com/bitcoin/bitcoin/pull/29215/commits/931575418e082af37b88b1819125b0d0f0fabbd5
Tested locally. Thanks, stackman!
💬 theuni commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447482492)
Looks good, thanks. Looking at 7b00595d335915dc2bf856e3569115996381a402 it's clear that this is what was intended.
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447482492)
Looks good, thanks. Looking at 7b00595d335915dc2bf856e3569115996381a402 it's clear that this is what was intended.
💬 theuni commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885015028)
Re libtool: As far as I can see, it's only used for miniupnpc/libnatpnp?
If that's the case, I don't think it's worth the trouble of having users work around it.
I'd suggest we either:
- patch out `libtool` in the Makefiles and use `ar`, then drop our `libtool` machinery entirely.
- Switch to their cmake builds where I assume `libtool` is unused.
I have a [branch with commits to do the latter](https://github.com/theuni/bitcoin/commits/depends-cmake/) as part of a larger CMake switch,
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885015028)
Re libtool: As far as I can see, it's only used for miniupnpc/libnatpnp?
If that's the case, I don't think it's worth the trouble of having users work around it.
I'd suggest we either:
- patch out `libtool` in the Makefiles and use `ar`, then drop our `libtool` machinery entirely.
- Switch to their cmake builds where I assume `libtool` is unused.
I have a [branch with commits to do the latter](https://github.com/theuni/bitcoin/commits/depends-cmake/) as part of a larger CMake switch,
...
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885034377)
> Edit: If there's interest in the above I can PR it separately.
Sure, I can rebase on top of that PR, if you want to open it.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885034377)
> Edit: If there's interest in the above I can PR it separately.
Sure, I can rebase on top of that PR, if you want to open it.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447390077)
In "refactor: Add a pre-mutexed version of IsPeerRegistered":
I'd prefer to swap the names (= have an external `IsPeerRegistered`, and an internal `IsPeerRegisteredInternal`), as external callers ought to only care about the external one. Also is it possible to make the internal one `private`?
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447390077)
In "refactor: Add a pre-mutexed version of IsPeerRegistered":
I'd prefer to swap the names (= have an external `IsPeerRegistered`, and an internal `IsPeerRegisteredInternal`), as external callers ought to only care about the external one. Also is it possible to make the internal one `private`?