💬 willcl-ark commented on issue "Add `consolidate_wallet` argument to fund* RPCs":
(https://github.com/bitcoin/bitcoin/issues/24795#issuecomment-2047858044)
The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.
(https://github.com/bitcoin/bitcoin/issues/24795#issuecomment-2047858044)
The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.
💬 maflcko commented on issue "valgrind: Conditional jump or move depends on uninitialised value(s)":
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-2047861599)
Some options:
* Add a valgrind suppression
* Avoid clang17+ for now, use up to clang16 in CI
* Something else?
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-2047861599)
Some options:
* Add a valgrind suppression
* Avoid clang17+ for now, use up to clang16 in CI
* Something else?
💬 maflcko commented on pull request "refactor: Simplify base32/64 padding calculations":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2047875411)
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:
* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.
For more in
...
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2047875411)
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:
* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.
For more in
...
💬 paplorinc commented on pull request "refactor: Simplify base32/64 padding calculations":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2047973653)
Thanks for the review @fanquake and @maflcko - though it was still in draft form because of the unsigned problems.
I didn't mean the change as a stylistic change.
In the encodings the original `str.reserve` doesn't enforce the final size, but after the change it's obvious that it's an exact calculation, not just an upper bound, i.e. `while (str.size() % 4) str += '=';` vs `str.append(size - str.size(), '=');`.
In the second change (which I've removed for now) the point was to unify the
...
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2047973653)
Thanks for the review @fanquake and @maflcko - though it was still in draft form because of the unsigned problems.
I didn't mean the change as a stylistic change.
In the encodings the original `str.reserve` doesn't enforce the final size, but after the change it's obvious that it's an exact calculation, not just an upper bound, i.e. `while (str.size() % 4) str += '=';` vs `str.append(size - str.size(), '=');`.
In the second change (which I've removed for now) the point was to unify the
...
💬 itornaza commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2048105785)
> Reasonable minds can differ of course, for future readers of the PR leave a comment or give me a 👎 if you disagree and would like to see the default parameter used in this case. If it is clear that I'm in the minority, I will change it.
Who am I to say, but I philosophically agree with your reasoning on not passing default parameters in critical test code.
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2048105785)
> Reasonable minds can differ of course, for future readers of the PR leave a comment or give me a 👎 if you disagree and would like to see the default parameter used in this case. If it is clear that I'm in the minority, I will change it.
Who am I to say, but I philosophically agree with your reasoning on not passing default parameters in critical test code.
👋 Eunovo's pull request is ready for review: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680)
(https://github.com/bitcoin/bitcoin/pull/29680)
💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1560028640)
The error message is:
```
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 406, in <module>
main()
File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 110, in main
test_list_all = parse_test_list(
^^^^^^^^^^^^^^^^
File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 392, in parse_test_list
test_list_all = subprocess.run(
^^^^^^^^^^^^^^^
File "C:\hostedtoolcache\windows
...
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1560028640)
The error message is:
```
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 406, in <module>
main()
File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 110, in main
test_list_all = parse_test_list(
^^^^^^^^^^^^^^^^
File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 392, in parse_test_list
test_list_all = subprocess.run(
^^^^^^^^^^^^^^^
File "C:\hostedtoolcache\windows
...
💬 hebasto commented on pull request "guix: replace GCC unaligned VMOV patch with binutils patch":
(https://github.com/bitcoin/bitcoin/pull/29846#issuecomment-2048425728)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/29846#issuecomment-2048425728)
Concept ACK.
💬 paplorinc commented on pull request "refactor: Simplify base32/64 padding calculations":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2048485814)
@maflcko, you were right, the changes didn't add enough value, I've reverted them.
I've kept the tests, since I think they're valuable and unified the style between base 32 and 64 in the source. If you think that's still too much, I'll revert it.
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2048485814)
@maflcko, you were right, the changes didn't add enough value, I've reverted them.
I've kept the tests, since I think they're valuable and unified the style between base 32 and 64 in the source. If you think that's still too much, I'll revert it.
💬 niftynei commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-2048493284)
Yes but won't have any progress to show til May or so. Thanks
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-2048493284)
Yes but won't have any progress to show til May or so. Thanks
👋 paplorinc's pull request is ready for review: "refactor/test: add a few more base32/64 calculation corner cases"
(https://github.com/bitcoin/bitcoin/pull/29847)
(https://github.com/bitcoin/bitcoin/pull/29847)
💬 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?