💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1560567478)
Linux does not need the `COMSPEC` environment variable to spawn a new process, right?

💬 maflcko commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1560573673)
Ok, then maybe mention that this is needed for `COMSPEC` in the commit message?

📝 fanquake opened a pull request: "ci: use Clang 16 for Valgrind"
(https://github.com/bitcoin/bitcoin/pull/29848)
Switch to Ubuntu Noble.
Valgrind 3.19 -> 3.22
Clang 14 -> Clang 16

👍 theStack approved a pull request: "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure"
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1993588649)
ACK 4ba1d0b55339c3ea90e2bcd64662a06f0f90dd46

💬 dergoegge commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2049209648)
`package_rbf` crash (base64): [rbf-2369c110673171b822af54ba3f33aabab58e7aeb.crash.txt](https://github.com/bitcoin/bitcoin/files/14943549/rbf-2369c110673171b822af54ba3f33aabab58e7aeb.crash.txt)

`util/feefrac.cpp:44 CompareChunks: Assertion slope_ap.size > 0 failed.`

💬 maflcko commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2049211637)
post-merge NACK. Can you add exact steps to reproduce the debug mode works as expected?

💬 dergoegge commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2049220195)
UBSan `package_rbf` crash (base64):
[rbf-ubsan-fac11eca92adf89df0e6840e5faa58873144d98a.crash.txt](https://github.com/bitcoin/bitcoin/files/14943698/rbf-ubsan-fac11eca92adf89df0e6840e5faa58873144d98a.crash.txt)


```
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3880912967
INFO: Loaded 1 modules (756212 inline 8-bit counters): 756212 [0xaaaadcf1c108, 0xaaaadcfd4afc),
INFO: Loaded 1 PC tables (756212 PCs): 756212 [0xaaaadcfd4b00,0xaaaaddb5ea40),
/workdir/fuzz_bin
...

💬 maflcko commented on pull request "ci: use Clang 16 for Valgrind":
(https://github.com/bitcoin/bitcoin/pull/29848#issuecomment-2049230133)
lgtm ACK 9996fba412f2d6d0e899906e8b3c387ef9d199d3

Didn't test

💬 maflcko commented on pull request "add `-limitdummyscriptdatasize` option":
(https://github.com/bitcoin/bitcoin/pull/29520#discussion_r1560687230)
I don't think script.h is the right place to put policy code. Anything in this file should be considered consensus-critical. My understanding is that all functions in this class are consensus-critical, and adding a new one that isn't does not seem ideal. See for example commit 87fe71e1fc810ee120a10063fdd26c3245686d54 for some historical context. It wouldn't be a great outcome if a future change to this function or functionality caused a consensus change and chain split.

I'd say that pure poli
...

🤔 ismaelsadeeq reviewed a pull request: "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure"
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1993669414)
re-ACK https://github.com/bitcoin/bitcoin/commit/4ba1d0b55339c3ea90e2bcd64662a06f0f90dd46 via [diff](https://github.com/bitcoin/bitcoin/compare/4fe7d150eb3c85a6597d8fc910fe1490358197ad..4ba1d0b55339c3ea90e2bcd64662a06f0f90dd46)

💬 maflcko commented on issue "RFC: "Insufficient review" tag for closed PRs":
(https://github.com/bitcoin/bitcoin/issues/29839#issuecomment-2049258564)
I agree that it should be added, but I think there is an issue that not everyone is happy to NACK a pull request, so instead they'll wait for the pull request to go stale and be closed "naturally" to avoid having to spend time to NACK it.

Just something to watch out for when applying the label liberally, but otherwise sgtm.

💬 GBKS commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2049267137)
Not related to the review of this PR, but I thought I'd follow up on my previous comment (let me know if you don't want me to do this). For the Bitcoin Core App, I am proposing to have show the verification status in the text field that we also use for the date and other statuses in transactions. Users can always click a transaction to see the date if they really need to. This keeps the list simple to parse, which is especially important on mobile. The general verification status is indicated in
...

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2049289633)
Please make sure to re-read the whole comment I shared and make sure to read all pages it links to. The burden to clearly motivate a change (and the review effort it requires) is on the pull request author (in this case here it would be you).

Open Source does not work by doing random unmotivated changes, opening a pull request, seeing it fail CI and tests, then adjust it in a ping-pong fashion to make CI green and go back and forth with reviewers until they have essentially written the whole
...

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2049293847)
@maflcko, which of the listed motivation/review/passing CI doesn't apply here?

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2049354132)
All of it.

* "unified ... code" is not a motivation, but a style choice
* The CI didn't pass on several tries
* Review doesn't seem worth it, unless there is a motivation

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1560771809)
This is the wrong style in any case, see the dev notes

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1560773944)
I can reorder them if you think it's worth it.
If you think these contributions are worthless I'll stop.

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1560776211)
> I can reorder them if you think it's worth it.

I don't understand what you are trying to say.

I am saying that the order was correct before and you changed it to be wrong.

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1560780178)
I added a new import and the imports were reorganized as such. If you don't like the order, I can change it, I don't mind, though I would prefer the CI telling me these in the PR instead of wasting time reviewing styles.

💬 sipa commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2049368065)
@dergoegge Do those fuzz seeds trigger issues on master too?