💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192264695)
It seems like it would be more natural to write this as:

```c++
class ChainstateNotifications
{
virtual void NotifyBlockTip(SynchronizationState state, CBlockIndex* index) {}
virtual void NotifyHeaderTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {}
virtual void ShowProgress(const std::string& title, int nProgress, bool resume_possible) {}
virtual void DoWarning(const bilingual_str& warning) {}
virtual void InitError(const bilingual
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192295222)
I don't think I would tie these notifications to the ChainStateManager class specifically as they really make sense anywhere in validation code. Would suggest changing `chainstatemanager_notifications` to `chainstate_notifications`, or `validation_notifications` and dropping the "manager" in these places

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1192301612)
Yes, and maybe also send `wtxidrelay`. Any reason not to send `TX` directly and avoid the `INV`, like below?

* VERSION
* [after receiving VERSION] WTXIDRELAY VERACK
* [after receiving VERACK] TX
* PING
* [after receiving PONG] disconnect

👍 fanquake approved a pull request: "build: Fix shared lib linking for darwin with lld"
(https://github.com/bitcoin/bitcoin/pull/27628#pullrequestreview-1424424089)
ACK d576d69e139ba5724c87d405d6161dc062ddc6f7 - also tested via 21778

💬 fanquake commented on pull request "build: Include qt sources for parsing with extract_strings.py":
(https://github.com/bitcoin/bitcoin/pull/22764#issuecomment-1545675599)
@jarolrod want to followup here?

💬 ryanofsky commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192306671)
> nit: document those as the return value may not be so obvious to everybody: "if a non-empty optional is returned then an error occurred and it contains the error message".

It's good to document, but this convention does seem to be increasingly used. I have a PR to replace `std::optional<bilingual_str>` with `util::Result` various places (74655e5870888e0165c62fec75000fe04f062147, #25977)

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1545683197)
Thanks all for the feedback!

> Just to clarify: We would open the short-lived connection to relay our own transaction and close it right after and then, we wait until we receive an INV about that tx from somebody, if not, we would try to do it (open a new short-lived conn...) all over again?

Yes. But we may as well send to a few peers at the same time, then wait. And, I am not sure, maybe if it does not work for a long time, then fall back to the existent mechanism?

The worst that can h
...

💬 furszy commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1545695358)
> Does this PR makes
>
> https://github.com/bitcoin/bitcoin/blob/137a98c5a22e058ed7a7997a0a4dbd75301de51e/test/sanitizer_suppressions/tsan#L28
>
> outdated?

@hebasto hmm, interesting. It seems to be outdated in master. Just ran the test suite, without the suppression, in master and it passed.

✅ MarcoFalke closed an issue: "Script verification being run when rebuilding UTXO database."
(https://github.com/bitcoin/bitcoin/issues/27639)

💬 MarcoFalke commented on issue "Script verification being run when rebuilding UTXO database.":
(https://github.com/bitcoin/bitcoin/issues/27639#issuecomment-1545695645)
You can leave `-assumevalid`'s default value to skip the checks. However, if you run into disk corruption, I'd recommend to disable it via `-assumevalid=0` to catch problems in files on disk.

💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1192328933)
good. Not sure if there is any but would be nice to have coverage for the automatic re-submission. Would just need to mock the node time 24hs in the future and see if they get re-submitted.

💬 vasild commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192332228)
Using `util::Result` here would be self-documenting.

🤔 hebasto reviewed a pull request: "Introduce field element and group element classes to test_framework/key.py"
(https://github.com/bitcoin/bitcoin/pull/26222#pullrequestreview-1424459844)
> To maximize readability...

Concept ACK on that. At least, non-cryptography abstract algebra background is enough to review the code :)

Partially reviewed. The `class FE` looks good.

💬 ryanofsky commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545703548)
> > I don't think it's useful to have these kind of PRs
>
> Ok, this is your opinion. Sometimes I feel the same for some PRs. My reaction is then to ignore them - don't spend _my_ time on them if _I_ think it is not worth it. But I don't try to impose my opinion on others

I agree with you that at a project / maintenance level we should be neutral about "I don't think this PR is a good use of effort but I can't point out any actual problems with it" review feedback. If a PR has sufficient r
...

💬 ryanofsky commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545712600)
> The code comment needs to be fixed as it is now incorrect, and the naming is now misleading. I think the change is clearly justified here.

Could you say more about how the current code is misleading in the PR description? I'm sure the new code is better, but knowing what is incorrect or misleading in the current code could help motivate the PR a little more

💬 MarcoFalke commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1192361975)
> Even then this approach seems like it can trigger false positives fairly easily, since the rolling bloom filter capacity is only 3500 txs?

I didn't understand this. How is this different from a peer connecting and waiting for 3500 "normal" newly announced transaction to be announced and added to the bloom filter to achieve the same false positive rate?

> or since it's a rolling bloom filter, causing overflow leading to false negatives (leading to functionality bugs where you can't get th
...

💬 Sjors commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1545737922)
Partial utACK for the changes in `CompareDepthAndScore`. Took me a while to wrap my head around it.

💬 instagibbs commented on pull request "test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes #27595)":
(https://github.com/bitcoin/bitcoin/pull/27631#issuecomment-1545744936)
IIUC the function of that data push is just to "randomize" the sighash, so I think dropping size 1 from the range is reasonable, maybe leave a comment as such if my intuition is correct?

📝 MarcoFalke opened a pull request: "test: Return dict in MiniWallet::send_to"
(https://github.com/bitcoin/bitcoin/pull/27640)
Returning a tuple has many issues:

* If only one value is needed, it can not be indexed by name
* If another value is added to the return value, all call sites need to be updated

Byte the bullet now and update all call sites to fix the above issues.

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1192388557)
done