💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1666620945)
I deleted the logs mentioned.

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2210605752)
I'm not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request? The first two commits seem fairly mechanical, but the last one less so. The comments in the code are helpful, but some pointers in the PR description for what to look out for, or a broad description of the approaches taken would be helpful too. E.g. why is so much custom functionality introduced for `findBlock`?

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1666625190)
Thanks @mzumsande. Makes sense.

I removed the third commit since there is agreement that it is redundant with `test_snapshot_with_less_work()`.

> Unrelated: @alfonsoromanz Thanks for the mention, but could you remove the "@" from my name in the PR description - if it'd get merged with it, I'll get notifications whenever some altcoin fork cherrypicks that commit later.

Sorry about that. The "@" was already removed by fanquake.

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2210613111)
I removed the third commit (https://github.com/bitcoin/bitcoin/commit/af0f401258e0c189799a36f4487eaa751d779e7b) since there is agreement that it is redundant with `test_snapshot_with_less_work()` (https://github.com/bitcoin/bitcoin/pull/29428). See discussion: https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665838469

💬 naiyoma commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2210623450)
Concept ACK.
Nit: IMO, [https://github.com/bitcoin/bitcoin/commit/64d35be20c43f764e5c22657085a790d0adcfe22](https://github.com/bitcoin/bitcoin/commit/64d35be20c43f764e5c22657085a790d0adcfe22) message should be amended to also include `nTxCount` and `nTxDiff`, since it's not just `nChainTx` that's changing to uint64_t.

💬 maflcko commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1666644819)
I wrote a simple patch to make the existing `Result` more flexible (allow inner error types other than `bilingual_str`), for places that want to use something like `std::expected<A, B>` in code today by using `Result<A, B>`.

I understand that the `ErrorString` helper won't be working as-is anymore. But that seems fine, because a standalone function can be provided to turn `B` into a string.

I also understand that the approach does not allow for multiple failures, errors, and warnings.

N
...

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1666649324)
Perhaps slightly so, because here at least `setup_bytes()` isn't called again.

My main point was that with `InitScriptExecutionCache` it's clearly an initialization function that's affecting global state. On the other hand, one shouldn't have to expect that simply constructing an object is going to invalidate all other objects of the same type, that's unintuitive and rather opaque I think.

But we can leave this as is, it's resolved in the next commit after all.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1666663060)
will do if I retouch

💬 dergoegge commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1666684208)
Creating the sig cache every iteration (instead of the global) would be better

💬 willcl-ark commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1666695881)
Thanks @furszy, this indeed feels unclean to me.

I made a few changes in another test branch and I wonder if they make sense to you: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:walletprocesspsbt-no-finalize-fixups

I took inspiration from https://github.com/furszy/bitcoin-core/commit/698e090ead3d0603146c98bd2ec06e44389f34cc (thanks) but it seems that I can probably just clear the entire output map as done in [L94](https://github.com/bitcoin/bitcoin/compare/master.
...

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1666706287)
`Entry` struct does not have any other distinguishing information, are you suggesting adding txid to `Entry`?

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1666706550)
which order?

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1666757944)
Should we ideally check both?

🤔 furszy reviewed a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2160642658)
ACK 46819f5df6de2a860c3ec87d55527b617a3b128f

💬 maflcko commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1666789017)
The reason to use `std::move` in `ChainstateManager` was not to optimize for anything, but to get the tidy-error for use-after-move, because using the options un-flattened may later on lead to an assertion error or other runtime error.

💬 maflcko commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2210854266)
pm-lgtm

💬 fjahr commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#issuecomment-2210856839)
utACK fa22edc1ef4d4456dcde883acae6f38af142d7bf

💬 furszy commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1666805504)
> I made a few changes in another test branch and I wonder if they make sense to you: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:walletprocesspsbt-no-finalize-fixups

The question here is whether walletprocesspsbt should update the existing PSBT 'outputs' information (similar to how it updates the transaction inputs) or should only append data to it as it is currently doing. In the current test form, the first entry of the PSBT 'outputs' field ends up with two BIP32
...

💬 andrewtoth commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2210899073)
> disabling unrelated validations

I don't think @sipa said to disable validations. "benchmark pre-assumevalid IBD" means testing IBD (or reindex) before the assumevalid block, which right now is block 824,000. By setting `-assumevalid=0` that means you disabled using assumevalid entirely, so you will do signature checks from genesis.

> limiting `-stopatheight=200000`

With this limit you should not see any benefit from this change. Using default `dbcache` settings the cache is not flushe
...

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1666830623)
Yes, ideally in a separate, new test.