💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824176187)
I'd say it would be easier to sell to upstream if it was checked at compile-time at no runtime cost/overhead. Is there a benefit of doing it at runtime?

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824185306)
It might add extra friction to agree on preprocessor logic to make the code still compile on other compilers. Also, doing it at compile-time means it's an all/nothing thing that can't be turned off for individual sloppy tests. But I agree it would be great to see if upstream would take a compile-time version first.

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824197377)
> agree on preprocessor logic

Maybe I am missing something, but it should be pretty standard, similar to

https://github.com/bitcoin/bitcoin/blob/f07a533dfcb172321972e5afb3b38a4bd24edb87/src/span.h#L22-L30

?

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824202809)
Yeah, hopefully shouldn't be much of an obstacle, maybe I'm too pessimistic about that aspect.

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2449494945)
> Makes me wonder if there is more historical context to this.

Yes, me too. If there are good reasons to keep these checks, then we should document why (and why they are not needed for the other interfaces). I could not come up with one, so I opened this PR, but I may well be missing something.

💬 maflcko commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449496369)
An easy first fix would be to downgrade `InitError` to `InitWarning`, as the alert shouldn't be fatal, as it is possible to continue without it. (This applies to bitcoind as well)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824208275)
I agree now, it's equivalent. Still wrapping my head around `RelayTransaction` refactoring :)

---------------------------

A difference could be in handling dependencies: during the trickle window after the first transaction arrives.

Say, a parent arrives, and then a child arrives in a second. In my approach we may notice the child when handling (announcement-wise) the parent (because handling the parent is postponed via trickle).
In your approach, you handle the parent independently.

...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824212857)
another question: say a child arrives a few minutes after a parent (parent still in the mempool, but it's been announced to peers a while ago). Why bother with this inv? Perhaps we should do it only if `TryRemovingFromSet` actually have removed it?

💬 maflcko commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2449507386)
Can you add a dummy commit to trigger the CI, so that the flag difference can be seen, and to confirm that there are no unwanted interactions?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824219529)
Apparently I was wrong.

```
// If the transaction fails because it collides with an existing one,
// we also remove and fanout the conflict and all its descendants.
// This is because our peer may have added the conflicting transaction
// to its set, in which reconciliation of these two would fail
```

Say #1, #2, #3 arrive.
- First #1 added to the set.
- then #2 doesn't make it to the set, but also drops #1 from it.
-
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824221603)
yes

👍 hodlinator approved a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2407594484)
ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440

range-diff shows all notable changes since my [Concept ACK](https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2397249971) are in response to my suggestions.

Ran each new fuzz target with:
```
₿ FUZZ=<target name> build_fuzz/src/test/fuzz/fuzz -max_total_time=30 -jobs=8
```
Also ran:
```
₿ ctest -j<X> --test-dir build --output-on-failure -R base
```

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2449539386)
Rebased due to the conflict with the merged bitcoin/bitcoin#31015.

🤔 maflcko reviewed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2407602211)
No strong opinion on the removals. I think the doc update is valuable and could be extended. Simply declaring the removed benchmarks as potentially not valuable, when looked at too narrowly, could be less controversial and still achieve the essence of the goal of this pull request.

💬 maflcko commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1824235336)
Maybe add a note to say that performance optimizations of micro-benchmarks are not valuable, if they do not show a visible end-user end-to-end performance improvement? Also could mention that the contribution of benchmarks that do not cover an end-user end-to-end scenario may not be valuable?


Even if they do, I think some improvements may still be rejected, if the code bloat or review/maintenance is too much to justify the improvement.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824243351)
I've checked once more. It fails without patching: https://cirrus-ci.com/task/4951518795792384.

💬 hebasto commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1824250163)
This `"$comment"` field actually comments the following line:https://github.com/bitcoin/bitcoin/blob/fad685ce54d39a333a6c75641b5d519cb0345959/vcpkg.json#L3-L4

💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1824271501)
Saw that, but it mentions it by name so I thought maybe keeping the lexicographical order the first commit tries to introduce would have precedent.

💬 hebasto commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1824280283)
I've dropped mentioning of lexicographical order from the commit message. The goal of the reordering is to minimize diffs for invocations like `vcpkg add port libqrencode`.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824285323)
Nit

```
const size_t removed = m_local_set.erase(wtxid) + m_delayed_local_set.erase(wtxid);
// Data must be in one of the sets at most
Assume(removed <= 1);
return removed == 1;
```