💬 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`?
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447509119)
In commit "p2p: Add transactions to reconciliation set"
This value could be cached inside `TxReconciliationTracker::Impl` I think, and updated on registering/deregister, to avoid recomputating it every time?
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447509119)
In commit "p2p: Add transactions to reconciliation set"
This value could be cached inside `TxReconciliationTracker::Impl` I think, and updated on registering/deregister, to avoid recomputating it every time?
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447401962)
If this set may contain up to 3000 (=`MAX_SET_SIZE`) elements, it may be worth using an `std::unordered_set` instead (with something like `SaltedTxidHasher` as hasher).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447401962)
If this set may contain up to 3000 (=`MAX_SET_SIZE`) elements, it may be worth using an `std::unordered_set` instead (with something like `SaltedTxidHasher` as hasher).
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447521744)
In commit "Cache fanout candidates to optimize txreconciliation"
Since the cache entry `std::set<NodeId>` don't change until being replaced entirely, I believe it would be more efficient to use a `std::vector<NodeId>`, with the nodeids in sorted order. You can then use `std::binary_search` to query in that vector.
It'd be more efficient as `std::vector` has much better memory locality than `std::set`, and fewer indirections.
If you do this, can also just reuse the `best_peers` vector, a
...
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447521744)
In commit "Cache fanout candidates to optimize txreconciliation"
Since the cache entry `std::set<NodeId>` don't change until being replaced entirely, I believe it would be more efficient to use a `std::vector<NodeId>`, with the nodeids in sorted order. You can then use `std::binary_search` to query in that vector.
It'd be more efficient as `std::vector` has much better memory locality than `std::set`, and fewer indirections.
If you do this, can also just reuse the `best_peers` vector, a
...
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447502258)
In commit "p2p: Add transactions to reconciliation sets"
It doesn't actually matter whether we pick the lowest-`hash_key` entries or highest-`hash_key` entries to fanout to, I think? If so, can just use `std::sort(best_peers.begin(), best_peers.end());`, and drop the `cmp_by_key`.
Also, since you only care about the top `targets_size` results, I believe using `std::partial_sort` *may* be faster (but benchmark it).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447502258)
In commit "p2p: Add transactions to reconciliation sets"
It doesn't actually matter whether we pick the lowest-`hash_key` entries or highest-`hash_key` entries to fanout to, I think? If so, can just use `std::sort(best_peers.begin(), best_peers.end());`, and drop the `cmp_by_key`.
Also, since you only care about the top `targets_size` results, I believe using `std::partial_sort` *may* be faster (but benchmark it).