💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1196923956)
> The confirmation counts for the UTXOs can be very outdated when blocks are generated by things other than the MiniWallet. It would be nice if these could be kept up to date so that this filter is accurate.
So, can I update `get_utxos` as well?
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1196923956)
> The confirmation counts for the UTXOs can be very outdated when blocks are generated by things other than the MiniWallet. It would be nice if these could be kept up to date so that this filter is accurate.
So, can I update `get_utxos` as well?
🤔 mzumsande reviewed a pull request: "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1431340479)
As I suggested in the review club, an alternative, more simple approach would be to just pick a random peer after removing NoBan and outbound connections when in force-mode. Then, if at the end of the usual eviction algorithm, we don't have a evict-able peer, we would evict the random one instead.
This would make the code simpler (no need to change `EraseLastKElements` or keep track of `last`), and I don't really see a major downside:
* The `EraseLastKElements` eviction criteria don't really s
...
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1431340479)
As I suggested in the review club, an alternative, more simple approach would be to just pick a random peer after removing NoBan and outbound connections when in force-mode. Then, if at the end of the usual eviction algorithm, we don't have a evict-able peer, we would evict the random one instead.
This would make the code simpler (no need to change `EraseLastKElements` or keep track of `last`), and I don't really see a major downside:
* The `EraseLastKElements` eviction criteria don't really s
...
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1551909232)
Updated 0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e -> a6612baf6aed92f74f96fa4bc04d0ee359f5cc3f ([chainstateRmKernelUiInterface_6](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_6) -> [chainstateRmKernelUiInterface_7](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_6..chainstateRmKernelUiInterface_7)).
* Addressed @ryanofsky's [comment](https://gi
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1551909232)
Updated 0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e -> a6612baf6aed92f74f96fa4bc04d0ee359f5cc3f ([chainstateRmKernelUiInterface_6](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_6) -> [chainstateRmKernelUiInterface_7](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_6..chainstateRmKernelUiInterface_7)).
* Addressed @ryanofsky's [comment](https://gi
...
💬 sipsorcery commented on pull request "msvc: Rename `libbitcoinconsensus` to `libbitcoin_consensus` and other adjustments":
(https://github.com/bitcoin/bitcoin/pull/27615#issuecomment-1551931568)
As the rename makes the naming more correct seems reasonable to me.
ACK a94d75fa81bab8f4695ab1756524e639af0ff69c.
(https://github.com/bitcoin/bitcoin/pull/27615#issuecomment-1551931568)
As the rename makes the naming more correct seems reasonable to me.
ACK a94d75fa81bab8f4695ab1756524e639af0ff69c.
💬 furszy commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1551934377)
Rebased post #25193 merge. Conflicts solved.
Only change from the last push is on the first commit 594031d, where the index threads active wait and the global flag are replaced by a post-poned indexers start call.
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1551934377)
Rebased post #25193 merge. Conflicts solved.
Only change from the last push is on the first commit 594031d, where the index threads active wait and the global flag are replaced by a post-poned indexers start call.
💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1196975954)
diff example:
```diff
index 0cde72c82..62a64b9dd 100644
--- a/test/functional/test_framework/wallet.py
+++ b/test/functional/test_framework/wallet.py
@@ -218,13 +218,12 @@ class MiniWallet:
txid: get the first utxo we find from a specific transaction
"""
self._utxos = sorted(self._utxos, key=lambda k: (k['value'], -k['height'])) # Put the largest utxo last
- mature_coins = list(filter(lambda utxo: not utxo['coinbase'] or COINBASE_MATURITY <= utxo['conf
...
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1196975954)
diff example:
```diff
index 0cde72c82..62a64b9dd 100644
--- a/test/functional/test_framework/wallet.py
+++ b/test/functional/test_framework/wallet.py
@@ -218,13 +218,12 @@ class MiniWallet:
txid: get the first utxo we find from a specific transaction
"""
self._utxos = sorted(self._utxos, key=lambda k: (k['value'], -k['height'])) # Put the largest utxo last
- mature_coins = list(filter(lambda utxo: not utxo['coinbase'] or COINBASE_MATURITY <= utxo['conf
...
💬 Sjors commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551970982)
> The commitment transaction may be as large as `MAX_STANDARD_TX_WEIGHT`, so we want the cluster size to allow that (but it's obvious that the limit needs to be higher than that anyway).
`MAX_STANDARD_TX_WEIGHT` is 400,000 weight units. If 65 byte transactions are (made) standard, and assuming those are at least 100 weight units, that's a maximum of ~4000 per cluster. My understanding was that doing an optimal sort for a given cluster, requires it to have a few dozen transactions. Though less
...
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551970982)
> The commitment transaction may be as large as `MAX_STANDARD_TX_WEIGHT`, so we want the cluster size to allow that (but it's obvious that the limit needs to be higher than that anyway).
`MAX_STANDARD_TX_WEIGHT` is 400,000 weight units. If 65 byte transactions are (made) standard, and assuming those are at least 100 weight units, that's a maximum of ~4000 per cluster. My understanding was that doing an optimal sort for a given cluster, requires it to have a few dozen transactions. Though less
...
💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1196990851)
this log line fires even if it actually failed?
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1196990851)
this log line fires even if it actually failed?
💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1196993686)
Seems ok, but would have to think about this more. I'd be ok with longer too, to start.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1196993686)
Seems ok, but would have to think about this more. I'd be ok with longer too, to start.
💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1196995104)
checking for log is good, but checking it was actually written is better in case there's a bug in the logging logic
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1196995104)
checking for log is good, but checking it was actually written is better in case there's a bug in the logging logic
💬 sipa commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551974824)
> My understanding was that doing an optimal sort for a given cluster, requires it to have a few dozen transactions.
It's the opposite really. A cluster with 1 transaction is always optimally ordered (because only one order exists). The bigger a cluster (in number of transactions) is, the harder it is to find the optimal ordering. Up to 15-20 transactions we may be able to find it usually an exponential-time algorithm; above that we need to either just use the ancestor-feerate based order,
...
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551974824)
> My understanding was that doing an optimal sort for a given cluster, requires it to have a few dozen transactions.
It's the opposite really. A cluster with 1 transaction is always optimally ordered (because only one order exists). The bigger a cluster (in number of transactions) is, the harder it is to find the optimal ordering. Up to 15-20 transactions we may be able to find it usually an exponential-time algorithm; above that we need to either just use the ancestor-feerate based order,
...
💬 sdaftuar commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551974872)
@t-bast Thanks for your comments!
> The commitment transaction may be as large as MAX_STANDARD_TX_WEIGHT, so we want the cluster size to allow that (but it's obvious that the limit needs to be higher than that anyway).
FYI -- I was thinking that the easiest approach here would be if we can cap cluster vbytes to the ancestor/descendant size limits of 101kvB, or just above the max standard transaction weight. This would have the benefit of not making block construction any less efficient (d
...
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551974872)
@t-bast Thanks for your comments!
> The commitment transaction may be as large as MAX_STANDARD_TX_WEIGHT, so we want the cluster size to allow that (but it's obvious that the limit needs to be higher than that anyway).
FYI -- I was thinking that the easiest approach here would be if we can cap cluster vbytes to the ancestor/descendant size limits of 101kvB, or just above the max standard transaction weight. This would have the benefit of not making block construction any less efficient (d
...
💬 Sjors commented on issue "Use muhash for assumeUTXO snapshot ":
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1551982585)
> However, a rolling UTXO set hash is incompatible with assumeutxo commitment schemes that involve chunking snapshots (discussed below) and so the resulting assumeutxo value might have to be a tuple consisting of `(rolling_set_hash, split_snapshot_chunks_merkle_root)`.
This is what I was getting at above. You just need a second hash that _is_ order dependent, which could be the sha256 hash of the `.dat` file, a torrent hash or the `split_snapshot_chunks_merkle_root`. I don't think that needs
...
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1551982585)
> However, a rolling UTXO set hash is incompatible with assumeutxo commitment schemes that involve chunking snapshots (discussed below) and so the resulting assumeutxo value might have to be a tuple consisting of `(rolling_set_hash, split_snapshot_chunks_merkle_root)`.
This is what I was getting at above. You just need a second hash that _is_ order dependent, which could be the sha256 hash of the `.dat` file, a torrent hash or the `split_snapshot_chunks_merkle_root`. I don't think that needs
...
💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1552010405)
@stickies-v @mzumsande thanks for youre review and feedback, I refactored the PR so we evict a random peer when forced if all other peers are protected. It is MUCH simpler ;-)
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1552010405)
@stickies-v @mzumsande thanks for youre review and feedback, I refactored the PR so we evict a random peer when forced if all other peers are protected. It is MUCH simpler ;-)
🤔 stickies-v reviewed a pull request: "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1431107177)
Light approach ACK c826187b5070ce89edcde0536183714a1c2e3207
I think this random selection approach suggested by mzumsande is a more elegant approach. "Light" in approach ACK because I need to build more confidence that this doesn't introduce new attack vectors.
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1431107177)
Light approach ACK c826187b5070ce89edcde0536183714a1c2e3207
I think this random selection approach suggested by mzumsande is a more elegant approach. "Light" in approach ACK because I need to build more confidence that this doesn't introduce new attack vectors.
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197024384)
This block can be skipped if `!force`
```suggestion
if (vEvictionCandidates.size() > 0 && force) {
```
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197024384)
This block can be skipped if `!force`
```suggestion
if (vEvictionCandidates.size() > 0 && force) {
```
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1196774113)
More than before this PR (thanks mzumsande for correcting my misunderstanding on IRC), the order in which these calls to erase elements are executed matters, as it directly affects which node is unprotected with `force==true`. Perhaps it's worth add a line of documentation to mention that the most important criteria need to be protected first?
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1196774113)
More than before this PR (thanks mzumsande for correcting my misunderstanding on IRC), the order in which these calls to erase elements are executed matters, as it directly affects which node is unprotected with `force==true`. Perhaps it's worth add a line of documentation to mention that the most important criteria need to be protected first?
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197030985)
Since `force_evict` is already an `optional` defaulting to `nullopt`, this can be simplified in conjunction with my other suggestion.
```suggestion
if (vEvictionCandidates.empty()) return force_evict;
```
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197030985)
Since `force_evict` is already an `optional` defaulting to `nullopt`, this can be simplified in conjunction with my other suggestion.
```suggestion
if (vEvictionCandidates.empty()) return force_evict;
```
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197025555)
nit:
```suggestion
size_t randpos{FastRandomContext().randrange(vEvictionCandidates.size())};
```
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197025555)
nit:
```suggestion
size_t randpos{FastRandomContext().randrange(vEvictionCandidates.size())};
```
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197031821)
Documenting the `force` parameter (or linking to `AttemptToEvictConnection`) would be helpful I think.
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197031821)
Documenting the `force` parameter (or linking to `AttemptToEvictConnection`) would be helpful I think.