💬 waketraindev commented on pull request "rpc,net: Add getpeerbyid and listpeersbyids RPCs":
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3071047468)
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
Yes, this is essentially a more complete version of that, incorporating feedback and suggestions from other contributors. It serves as an alternative approach to PR #32741.
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3071047468)
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
Yes, this is essentially a more complete version of that, incorporating feedback and suggestions from other contributors. It serves as an alternative approach to PR #32741.
🤔 furszy reviewed a pull request: "index: initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3017897441)
> Sorry, #32948 probably caused some conflicts.
Auch. Rebased.
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3017897441)
> Sorry, #32948 probably caused some conflicts.
Auch. Rebased.
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3071149725)
> It seems that since m_block_index is a std::unordered_map, the iteration order in RecalculateBestHeader is not guaranteed.
Oh yes, this bit me in an earlier iteration of this PR where I made the mistake to do `PickValue(fuzzed_data_provider, blockman.m_block_index)` directly to pick a random block index (so that no `blocks` vector was needed).
It's also related to the fact that we don't use `CBlockIndexWorkComparator` tiebreak rules for `m_best_header` (documented in #32843), so I think
...
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3071149725)
> It seems that since m_block_index is a std::unordered_map, the iteration order in RecalculateBestHeader is not guaranteed.
Oh yes, this bit me in an earlier iteration of this PR where I made the mistake to do `PickValue(fuzzed_data_provider, blockman.m_block_index)` directly to pick a random block index (so that no `blocks` vector was needed).
It's also related to the fact that we don't use `CBlockIndexWorkComparator` tiebreak rules for `m_best_header` (documented in #32843), so I think
...
👋 waketraindev's pull request is ready for review: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(https://github.com/bitcoin/bitcoin/pull/32972)
(https://github.com/bitcoin/bitcoin/pull/32972)
💬 tnndbtc commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3071393448)
I assume the general code logic should be correct, but likely some exception/run-time error occurred then caused the intermittent issue.
So, I studied `Popen::execute_process()` in src/util/subprocess.h, one interesting statement caught my attention:
```
subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below
stream_.close_child_fds();
```
I notice that we didn't check the return code of the calling of `subprocess_close()/close()`, in many cases. Indeed,
...
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3071393448)
I assume the general code logic should be correct, but likely some exception/run-time error occurred then caused the intermittent issue.
So, I studied `Popen::execute_process()` in src/util/subprocess.h, one interesting statement caught my attention:
```
subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below
stream_.close_child_fds();
```
I notice that we didn't check the return code of the calling of `subprocess_close()/close()`, in many cases. Indeed,
...
💬 andrewtoth commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3071549529)
> DUST_RELAY_TX_FEE should be decreased along with this as well...
That value is used to calculate the minimum output amount, not the fee rate of a tx. That seems orthogonal to the goal of this PR.
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3071549529)
> DUST_RELAY_TX_FEE should be decreased along with this as well...
That value is used to calculate the minimum output amount, not the fee rate of a tx. That seems orthogonal to the goal of this PR.
💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2206105512)
Ok, you are right on the `migratewallet` process, I've missed that, perhaps we can't go around that one (`CanSupportFeature(FEATURE_HD_SPLIT)`). The one in `getwalletinfo`, the condition itself can be removed, leaving the push of `keypoolsize_hd_internal`, as since legacy wallets can't no longer be loaded, can't run this command on them. But I doubt this is part of the scope of this PR, maybe can be done in a follow-up, like removing the rest of the functions (`GetVersion()`, `GetClosestWalletFe
...
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2206105512)
Ok, you are right on the `migratewallet` process, I've missed that, perhaps we can't go around that one (`CanSupportFeature(FEATURE_HD_SPLIT)`). The one in `getwalletinfo`, the condition itself can be removed, leaving the push of `keypoolsize_hd_internal`, as since legacy wallets can't no longer be loaded, can't run this command on them. But I doubt this is part of the scope of this PR, maybe can be done in a follow-up, like removing the rest of the functions (`GetVersion()`, `GetClosestWalletFe
...
🤔 l0rinc reviewed a pull request: "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries"
(https://github.com/bitcoin/bitcoin/pull/30673#pullrequestreview-3018067867)
The `src/coins.cpp` prod changes are a bit scary - while it's a very important change, based on the lack of enthusiasm by other, we may want to separate the tests and small refactors into PRs that will get us closer to this.
One could remove the invalid test states - without removing the invalid production code states, just potentially adding TODOs there in case it's missing.
The `SetFresh` and flag removal could be next and if all of those are merged, we can continue with the scary `FetchCo
...
(https://github.com/bitcoin/bitcoin/pull/30673#pullrequestreview-3018067867)
The `src/coins.cpp` prod changes are a bit scary - while it's a very important change, based on the lack of enthusiasm by other, we may want to separate the tests and small refactors into PRs that will get us closer to this.
One could remove the invalid test states - without removing the invalid production code states, just potentially adding TODOs there in case it's missing.
The `SetFresh` and flag removal could be next and if all of those are merged, we can continue with the scary `FetchCo
...
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2205983564)
nit: the class is already final + other overrides specify that explicitly for clarity
```suggestion
std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
```
and
```C++
bool HaveCoin(const COutPoint& outpoint) const override
```
And most other such methods in the file - but I understand if you'd prefer to do in a follow-up instead
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2205983564)
nit: the class is already final + other overrides specify that explicitly for clarity
```suggestion
std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
```
and
```C++
bool HaveCoin(const COutPoint& outpoint) const override
```
And most other such methods in the file - but I understand if you'd prefer to do in a follow-up instead
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206097629)
we don't have Fresh anymore, only `IsDirtyAndFresh`, right?
```suggestion
bool IsDirtyAndFresh() const noexcept
{
const bool is_fresh = m_flags & FRESH;
Assume(IsDirty() || !is_fresh);
return is_fresh;
}
```
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206097629)
we don't have Fresh anymore, only `IsDirtyAndFresh`, right?
```suggestion
bool IsDirtyAndFresh() const noexcept
{
const bool is_fresh = m_flags & FRESH;
Assume(IsDirty() || !is_fresh);
return is_fresh;
}
```
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206103568)
Given the lack of reviews, we may want to split out the test cleanups (removing invalid states) and refactors to a separate PR, where this will be a tracking PR for where we want to get to in safer/smaller steps
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206103568)
Given the lack of reviews, we may want to split out the test cleanups (removing invalid states) and refactors to a separate PR, where this will be a tracking PR for where we want to get to in safer/smaller steps
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206095812)
Since it's only called here, we could inline and simplify it now:
```suggestion
static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept
{
if (!pair.second.m_flags) {
Assume(!pair.second.m_prev && !pair.second.m_next);
pair.second.m_prev = sentinel.second.m_prev;
pair.second.m_next = &sentinel;
sentinel.second.m_prev = &pair;
pair.second.m_prev->second.m_next = &pair;
...
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206095812)
Since it's only called here, we could inline and simplify it now:
```suggestion
static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept
{
if (!pair.second.m_flags) {
Assume(!pair.second.m_prev && !pair.second.m_next);
pair.second.m_prev = sentinel.second.m_prev;
pair.second.m_next = &sentinel;
sentinel.second.m_prev = &pair;
pair.second.m_prev->second.m_next = &pair;
...
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206104394)
```suggestion
CCoinsCacheEntry::SetDirty(n2, sentinel, /*fresh*/true);
```
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206104394)
```suggestion
CCoinsCacheEntry::SetDirty(n2, sentinel, /*fresh*/true);
```
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206105073)
we should be able to do this safely in a prefactor PR regardless of the other changes
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2206105073)
we should be able to do this safely in a prefactor PR regardless of the other changes
💬 luke-jr commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2206206502)
?
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2206206502)
?
💬 luke-jr commented on pull request "test: Improve getbalance minconf behavior documentation and testing":
(https://github.com/bitcoin/bitcoin/pull/32974#issuecomment-3071776952)
Spent coins should never be included in the balance (though the change should be)
However, getbalance has basically been completely broken in Core since 2018. My last attempt to fix it "properly" was here: https://github.com/bitcoin/bitcoin/pull/14602 (but there were still bugs)
Knots has a mostly-functional getbalance by reverting the 2018 changes: https://github.com/luke-jr/bitcoin/tree/bugfix_rpc_getbalance_hacky
Happy to discuss at more length if you're up for fixing getbalance.
(https://github.com/bitcoin/bitcoin/pull/32974#issuecomment-3071776952)
Spent coins should never be included in the balance (though the change should be)
However, getbalance has basically been completely broken in Core since 2018. My last attempt to fix it "properly" was here: https://github.com/bitcoin/bitcoin/pull/14602 (but there were still bugs)
Knots has a mostly-functional getbalance by reverting the 2018 changes: https://github.com/luke-jr/bitcoin/tree/bugfix_rpc_getbalance_hacky
Happy to discuss at more length if you're up for fixing getbalance.
👍 stratospher approved a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-3018395027)
reACK 61e800e7.
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-3018395027)
reACK 61e800e7.
💬 l0rinc commented on pull request "test: Improve getbalance minconf behavior documentation and testing":
(https://github.com/bitcoin/bitcoin/pull/32974#issuecomment-3071807669)
> Spent coins should never be included in the balance
Spentness checks are still problematic all over coins caching as well - not sure it helps, but @andrewtoth attempted a cleanup in https://github.com/bitcoin/bitcoin/pull/30673
(https://github.com/bitcoin/bitcoin/pull/32974#issuecomment-3071807669)
> Spent coins should never be included in the balance
Spentness checks are still problematic all over coins caching as well - not sure it helps, but @andrewtoth attempted a cleanup in https://github.com/bitcoin/bitcoin/pull/30673
📝 l0rinc opened a pull request: "[IBD] log start of script validation past `assumevalid` block"
(https://github.com/bitcoin/bitcoin/pull/32975)
The `-assumevalid` option skips script verification for a specified block and all its ancestors during Initial Block Download.
Many first-time users are surprised when this suddenly slows their node to a halt.
This commit adds a log message to clearly indicate when this optimization ends and full validation begins.
The message is logged exactly once, when script validation is enabled **and** the previous block's hash matches the one set by [`-assumevalid`](https://github.com/bitcoin/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/32975)
The `-assumevalid` option skips script verification for a specified block and all its ancestors during Initial Block Download.
Many first-time users are surprised when this suddenly slows their node to a halt.
This commit adds a log message to clearly indicate when this optimization ends and full validation begins.
The message is logged exactly once, when script validation is enabled **and** the previous block's hash matches the one set by [`-assumevalid`](https://github.com/bitcoin/bitcoin
...
💬 PJJacobowitz commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3072000666)
Hey all, it looks like QT6.5 supports ARM64 in Windows. Does this mean there can be a Bitcoin Core that runs native on Windows on ARM?
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3072000666)
Hey all, it looks like QT6.5 supports ARM64 in Windows. Does this mean there can be a Bitcoin Core that runs native on Windows on ARM?