💬 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).
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447531912)
In commit "Cache fanout candidates to optimize txreconciliation"
Use `std::move` around `new_fanout_candidates` (or `best_peers` if you take my earlier suggestion to use a vector), to avoid a copy. If lookup the `peer_id` in it before moving, so you can return that value.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447531912)
In commit "Cache fanout candidates to optimize txreconciliation"
Use `std::move` around `new_fanout_candidates` (or `best_peers` if you take my earlier suggestion to use a vector), to avoid a copy. If lookup the `peer_id` in it before moving, so you can return that value.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447504347)
In commit "p2p: Add transactions to reconciliation set"
Would it make sense to have a helper function instead of `IsPeerRegistered`, that instead of returning a `bool` returns a `TxReconciliationState*` (`nullptr` if not found)? That would avoid locking/finding twice. I suspect it'd be usable in other places too.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447504347)
In commit "p2p: Add transactions to reconciliation set"
Would it make sense to have a helper function instead of `IsPeerRegistered`, that instead of returning a `bool` returns a `TxReconciliationState*` (`nullptr` if not found)? That would avoid locking/finding twice. I suspect it'd be usable in other places too.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447514047)
In commit "p2p: Add transactions to reconciliation set"
You could pick a fixed seed and wtxid for which you assert that with say 35 peers 3 are picked, and another seed/wtxid for which you assert that with 35 peers 4 are picked. That would at least show that the fractional probability code isn't just rounding down.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447514047)
In commit "p2p: Add transactions to reconciliation set"
You could pick a fixed seed and wtxid for which you assert that with say 35 peers 3 are picked, and another seed/wtxid for which you assert that with 35 peers 4 are picked. That would at least show that the fractional probability code isn't just rounding down.
👍 dergoegge approved a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1813583546)
tACK a0419151541fd5698cb4ddb4754395a22ec749a6
It might be better to reverse the order of the commits or squash them to avoid bisect failures in between.
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1813583546)
tACK a0419151541fd5698cb4ddb4754395a22ec749a6
It might be better to reverse the order of the commits or squash them to avoid bisect failures in between.
🤔 jamesob reviewed a pull request: "PoC: fuzz chainstate and block managers"
(https://github.com/bitcoin/bitcoin/pull/29158#pullrequestreview-1812003162)
Concept ACK; midway through review and trying to resolve some of the CI issues.
(https://github.com/bitcoin/bitcoin/pull/29158#pullrequestreview-1812003162)
Concept ACK; midway through review and trying to resolve some of the CI issues.
💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446610769)
When generating a valid block (here and below) do we want to do some kind of assertion that the block was actually considered valid? I could see the test code silently generating only invalid blocks and (afaik) there wouldn't be an indication of it here.
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446610769)
When generating a valid block (here and below) do we want to do some kind of assertion that the block was actually considered valid? I could see the test code silently generating only invalid blocks and (afaik) there wouldn't be an indication of it here.
💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1447615580)
https://github.com/bitcoin/bitcoin/pull/29158/commits/ea36af80beeeee0b9de793e52887ba3e164b803c
`fmemopen` is Linux-specific; here's a fixup commit that `ifdef`s it out and fixes some of the CI errors if you want it: https://github.com/jamesob/bitcoin/commit/26b9c9d48e3ec16e69d550266c7f27f4db9cb9f8
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1447615580)
https://github.com/bitcoin/bitcoin/pull/29158/commits/ea36af80beeeee0b9de793e52887ba3e164b803c
`fmemopen` is Linux-specific; here's a fixup commit that `ifdef`s it out and fixes some of the CI errors if you want it: https://github.com/jamesob/bitcoin/commit/26b9c9d48e3ec16e69d550266c7f27f4db9cb9f8
💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446611692)
This type comes up so often in this file that it might be worth an alias.
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446611692)
This type comes up so often in this file that it might be worth an alias.