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

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1666853094)
Good feedback. I've inverted the `if` condition (and branch). Hope it is clearer now?

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1666853871)
I'd say DB obfuscation is different from `*.dat` obfuscation, but I've made the messages a bit more similar. Thanks!

🤔 tdb3 reviewed a pull request: "[WIP] net: return result from addnode RPC"
(https://github.com/bitcoin/bitcoin/pull/30381#pullrequestreview-2160865613)
Concept ACK
There is definitely value in ensuring errors are provided to the user (e.g. no longer failing silently for unsuccessful `onetry`).

I could see there being value in returning the peer id, but think that might be better off for a follow up PR. This one could focus on fixing the problem of masking the error (and prevent discussion on a return object vs null from impacting what is otherwise a relatively straightforward bug fix).

If this PR remains scoped to returning the returnin
...