💬 laanwj commented on pull request "guix: replace GCC unaligned VMOV patch with binutils patch":
(https://github.com/bitcoin/bitcoin/pull/29846#issuecomment-2049044482)
Concept ACK, will test

💬 laanwj commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1560532624)
maybe "Accept was interrupted"

📝 fanquake converted_to_draft a pull request: "RPC: add new `listmempooltransactions`"
(https://github.com/bitcoin/bitcoin/pull/29016)
## Proposed Update

Add a new RPC endpoint, `listmempooltransactions`. Takes as args a `start_sequence` and `verbose`.

Returns:
- if verbose false, list of txids + their `entry_sequence` where each entry's `entry_sequence` >= the provided `start_sequence.
- if verbose true, raw tx output info including each entry's `entry_sequence`.


Builds on work done in #19572.

## Rationale
The current mempool RPCs are lacking an ability to scan for updates in a more efficient manner. You can
...

💬 maflcko commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1560540110)
I still fail to understand it. Can you explain what it means and why passing the env fixes it? Is there an error message on stderr or stdout as well?

💬 fanquake commented on pull request "guix: replace GCC unaligned VMOV patch with binutils patch":
(https://github.com/bitcoin/bitcoin/pull/29846#discussion_r1560542032)
It's so we build the whole world / toolchain / depends etc with this option.

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1560558840)
On Windows, when `subprocess` sets environment variables, it effectively overrides all of them, which makes `PATH` and `COMSPEC` environment variable empty on the master branch.

💬 maflcko commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1560562920)
Is it not the same on Linux?

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