💬 m3dwards commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2210562334)
Post merge Guix build matched:

```shell
d702d02df48bc540da55c47ca7110d122a27ba179ab728fb8bdb6e27589f754c guix-build-f59e9057e2aa/output/aarch64-linux-gnu/SHA256SUMS.part
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu.ta
...

💬 TheCharlatan commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#issuecomment-2210565170)
Getting a check failure during a guix build:
```
.
----------------------------------------------------------------------
Ran 1 test in 5.525s

OK
F
======================================================================
FAIL: test_ELF (__main__.TestSymbolChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/distsrc-base/distsrc-dae34eb480c8-arm-linux-gnueabihf/./contrib/devtools/test-symbol-check.py", line 69, in te
...

💬 naiyoma commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1666612849)
Would it make sense to also change this string `nchaintx` to `m_chain_tx_count` for consistency?

💬 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