💬 hebasto commented on pull request "guix: replace GCC unaligned VMOV patch with binutils patch":
(https://github.com/bitcoin/bitcoin/pull/29846#issuecomment-2049000540)
My Guix builds:
```
x86_64
07f97e7479e5a7e1baae091ed74839e9b7be118d0504a8b334c4add8e13c78cc guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/SHA256SUMS.part
ee4ab82a42c802dfa1e455e6badb6a49e819c77f03fcaf3568656faa5f69dce4 guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/bitcoin-a0dc2ebcda9e-aarch64-linux-gnu-debug.tar.gz
f405a168ea3aa2b3a230664381babb406b03680404ebb89e1638f07efa872520 guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/bitcoin-a0dc2ebcda9e-aarch64-linux-gnu.tar.gz
98c203bb
...
(https://github.com/bitcoin/bitcoin/pull/29846#issuecomment-2049000540)
My Guix builds:
```
x86_64
07f97e7479e5a7e1baae091ed74839e9b7be118d0504a8b334c4add8e13c78cc guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/SHA256SUMS.part
ee4ab82a42c802dfa1e455e6badb6a49e819c77f03fcaf3568656faa5f69dce4 guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/bitcoin-a0dc2ebcda9e-aarch64-linux-gnu-debug.tar.gz
f405a168ea3aa2b3a230664381babb406b03680404ebb89e1638f07efa872520 guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/bitcoin-a0dc2ebcda9e-aarch64-linux-gnu.tar.gz
98c203bb
...
💬 laanwj commented on pull request "guix: replace GCC unaligned VMOV patch with binutils patch":
(https://github.com/bitcoin/bitcoin/pull/29846#discussion_r1560502458)
i think i'm missing something: if this is a build flag, is a patch really needed? Or is this more of a defense-in-depth approach to prevent accidentally forgetting it.
(https://github.com/bitcoin/bitcoin/pull/29846#discussion_r1560502458)
i think i'm missing something: if this is a build flag, is a patch really needed? Or is this more of a defense-in-depth approach to prevent accidentally forgetting it.
💬 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
(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"
(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
...
(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?
(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.
(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.
(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?
(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?
(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?
(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
(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
(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.`
(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?
(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
...
(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
(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
...
(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)
(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.
(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.