💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205692104)
done, thanks

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205723116)
left it in for now.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691617)
added

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205722748)
added

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205719816)
CAUGHT 🔫

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691462)
added

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205735550)
Yeah it was originally implemented that way, but I think this way likely requires much fewer element lookups. Under "normal" conditions, we probably have multiple announcers for each transaction and there are very children from this peer.

🤔 mzumsande reviewed a pull request: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(https://github.com/bitcoin/bitcoin/pull/32972#pullrequestreview-3017872764)
> 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?

💬 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.

🤔 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.

💬 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
...

👋 waketraindev's pull request is ready for review: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(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,
...

💬 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.

💬 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
...

🤔 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
...

💬 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

💬 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;
}
```

💬 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

💬 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;

...