💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196834892)
> Not sure about this one. The include is required for setting the default argument and it is non-const, because we mutate the message.
That's a good point about the include. But changing to a `const bilingual_str&` reference and avoiding unnecessary copies would provide a more consistent interface.
For one thing, getting of the mutations would actually make current implementations of the interface more legible:
```c++
InitError(user_message.empty() ? _("A fatal internal error occurred
...
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196834892)
> Not sure about this one. The include is required for setting the default argument and it is non-const, because we mutate the message.
That's a good point about the include. But changing to a `const bilingual_str&` reference and avoiding unnecessary copies would provide a more consistent interface.
For one thing, getting of the mutations would actually make current implementations of the interface more legible:
```c++
InitError(user_message.empty() ? _("A fatal internal error occurred
...
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196840346)
Thanks for the follow-up!
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196840346)
Thanks for the follow-up!
👍 ryanofsky approved a pull request: "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/25193#pullrequestreview-1431213143)
Code review ACK 97844d9268b87b5d09b1091bfd0326ed18ce5b91. Just simple rebase since last review
(https://github.com/bitcoin/bitcoin/pull/25193#pullrequestreview-1431213143)
Code review ACK 97844d9268b87b5d09b1091bfd0326ed18ce5b91. Just simple rebase since last review
🚀 ryanofsky merged a pull request: "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/25193)
(https://github.com/bitcoin/bitcoin/pull/25193)
✅ ryanofsky closed an issue: "Coinstats index corrupted after invalidateblock and clean shutdown"
(https://github.com/bitcoin/bitcoin/issues/27558)
(https://github.com/bitcoin/bitcoin/issues/27558)
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196858330)
> If in the situation where an outbound (non-hb) peer is the first to announce that we still always send the getblocktxn if compact blocks fail, then this new logic would not be making anything worse, which seems sufficient to me.
Exactly. Fixed, I think!
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196858330)
> If in the situation where an outbound (non-hb) peer is the first to announce that we still always send the getblocktxn if compact blocks fail, then this new logic would not be making anything worse, which seems sufficient to me.
Exactly. Fixed, I think!
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196858467)
Looks like some lost in translation logic from prior PRs. Indeed, we can just peek at the first entry, if it exists. Fixed, I think.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196858467)
Looks like some lost in translation logic from prior PRs. Indeed, we can just peek at the first entry, if it exists. Fixed, I think.
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1551819825)
Thanks for the quick updates -- the code looks right to me now; will test.
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1551819825)
Thanks for the quick updates -- the code looks right to me now; will test.
💬 sinetek commented on issue "Option to prevent sleep":
(https://github.com/bitcoin/bitcoin/issues/27692#issuecomment-1551831106)
I don't think we should start messing with the user's sleep settings, it will just be confusing if every app did that. They can set themselves the power/conservative profile depending on their needs.
(https://github.com/bitcoin/bitcoin/issues/27692#issuecomment-1551831106)
I don't think we should start messing with the user's sleep settings, it will just be confusing if every app did that. They can set themselves the power/conservative profile depending on their needs.
💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1196911811)
@achow101 I agree on removing the fallback to use any UTXO when there are no mature coins. Going to address it.
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1196911811)
@achow101 I agree on removing the fallback to use any UTXO when there are no mature coins. Going to address it.
💬 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