💬 hebasto commented on issue "CMake-based build system tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-2413162020)
Closing. See https://github.com/bitcoin/bitcoin/pull/30454.
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-2413162020)
Closing. See https://github.com/bitcoin/bitcoin/pull/30454.
📝 l0rinc converted_to_draft a pull request: "CI: Add label to scripted-diffs"
(https://github.com/bitcoin/bitcoin/pull/31089)
Add a `scripted-diff` label for pull requests that contain `scripted-diff:` commits to make sure that CI is running them properly.
(https://github.com/bitcoin/bitcoin/pull/31089)
Add a `scripted-diff` label for pull requests that contain `scripted-diff:` commits to make sure that CI is running them properly.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1800679447)
This was introduced in b335710782c2545e6eeed67b5e1763c07eab26b0.
Related to https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1867610348.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1800679447)
This was introduced in b335710782c2545e6eeed67b5e1763c07eab26b0.
Related to https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1867610348.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1800702587)
Can you suggest another name? It's a map to get peer info, hence `m_peer_info`.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1800702587)
Can you suggest another name? It's a map to get peer info, hence `m_peer_info`.
💬 hebasto commented on issue "Run coverage functional tests in parallel":
(https://github.com/bitcoin/bitcoin/issues/26736#issuecomment-2413249267)
> This was probably resolved by `cmake`.
From the "Compiling for test coverage" section of [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-test-coverage):
> To enable test parallelism:
>
> cmake -DJOBS=$(nproc) -P build/Coverage.cmake
(https://github.com/bitcoin/bitcoin/issues/26736#issuecomment-2413249267)
> This was probably resolved by `cmake`.
From the "Compiling for test coverage" section of [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-test-coverage):
> To enable test parallelism:
>
> cmake -DJOBS=$(nproc) -P build/Coverage.cmake
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1800708031)
Grep will show `AddTxAnnouncement` called with `p2p_inv=false` in the new orphan handling code. We don't exit because we might already have a low feerate parent in reconsiderable rejects. You can also comment this out to see what fails.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1800708031)
Grep will show `AddTxAnnouncement` called with `p2p_inv=false` in the new orphan handling code. We don't exit because we might already have a low feerate parent in reconsiderable rejects. You can also comment this out to see what fails.
💬 laanwj commented on pull request "Don't zero-after-free `DataStream`: Faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2413262312)
Concept ACK. Using zero-after-free allocators for all CDataStream usage is overkill. i don't think there are any, but if there are places where it is used (wallet) where security against key leaks is important, we could parametrize `CDataStream<AllocatorType>` and pass a `secure_allocator`. Agree that zeroing adding extra security against buffer overflows is a red herring.
i expect this can make a significant differences on systems with relatively slow CPU or memory, like ARM systems (will r
...
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2413262312)
Concept ACK. Using zero-after-free allocators for all CDataStream usage is overkill. i don't think there are any, but if there are places where it is used (wallet) where security against key leaks is important, we could parametrize `CDataStream<AllocatorType>` and pass a `secure_allocator`. Agree that zeroing adding extra security against buffer overflows is a red herring.
i expect this can make a significant differences on systems with relatively slow CPU or memory, like ARM systems (will r
...
💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-2413278619)
Unfortunately limiting CPU and memory usage in Windows itself and in Project Lasso didn't work. Core still hangs while indexing after a few hours & the blocks are corrupted afterwards. It must be a hardware issue, as no one else experiences this issue.
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-2413278619)
Unfortunately limiting CPU and memory usage in Windows itself and in Project Lasso didn't work. Core still hangs while indexing after a few hours & the blocks are corrupted afterwards. It must be a hardware issue, as no one else experiences this issue.
💬 hebasto commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413296884)
> > Running Bitcoin Core on unsupported OSes may expose users to security issues.
> > macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
>
> If this is the reasoning we are going to use, shouldn't this be bumping to `10.13`, given `10.12` is also "unsupported" in terms of security updates?
You mean "13.0" and "12.0", right?
For the reference, macOS 12 was last updated on 2024-07-29. The three most recent Apple updates
...
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413296884)
> > Running Bitcoin Core on unsupported OSes may expose users to security issues.
> > macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
>
> If this is the reasoning we are going to use, shouldn't this be bumping to `10.13`, given `10.12` is also "unsupported" in terms of security updates?
You mean "13.0" and "12.0", right?
For the reference, macOS 12 was last updated on 2024-07-29. The three most recent Apple updates
...
💬 hebasto commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413322066)
If we raise the minimum supported macOS version to 13.0, it would mean dropping support for hardware like MacBook Pro (15-inch, 2016) and older, which Apple classifies as ["obsolete worldwide"](https://support.apple.com/en-us/102772).
I'll update this PR shortly.
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413322066)
If we raise the minimum supported macOS version to 13.0, it would mean dropping support for hardware like MacBook Pro (15-inch, 2016) and older, which Apple classifies as ["obsolete worldwide"](https://support.apple.com/en-us/102772).
I'll update this PR shortly.
💬 l0rinc commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1800757127)
I quickly glanced over this PR only, do I understand it correctly that we're treating benchmarks as tests here (I'm not sure I agree with that concept) and adding parameters to the benchmark setup that only apply to a smaller subset of the benchmarks?
Could we configure them via env variables instead and call the benchmark with e.g. `TEST_DATA_DIR=a/b/c bench` instead?
As an example, if a few of our tests require a for example a timestamp for whatever reason, we wouldn't add it to the testing
...
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1800757127)
I quickly glanced over this PR only, do I understand it correctly that we're treating benchmarks as tests here (I'm not sure I agree with that concept) and adding parameters to the benchmark setup that only apply to a smaller subset of the benchmarks?
Could we configure them via env variables instead and call the benchmark with e.g. `TEST_DATA_DIR=a/b/c bench` instead?
As an example, if a few of our tests require a for example a timestamp for whatever reason, we wouldn't add it to the testing
...
💬 maflcko commented on pull request "CI: Add label to scripted-diffs":
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2413340369)
Not sure what the goal here would be? Why add a label, when there is already a commit message prefix, which is identical in the meaning?
It would be better to just fix the bugs like https://github.com/bitcoin/bitcoin/issues/29692#issuecomment-2411656801 to ensure the CI is red when there is an issue.
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2413340369)
Not sure what the goal here would be? Why add a label, when there is already a commit message prefix, which is identical in the meaning?
It would be better to just fix the bugs like https://github.com/bitcoin/bitcoin/issues/29692#issuecomment-2411656801 to ensure the CI is red when there is an issue.
💬 maflcko commented on pull request "miniscript: convert non-critical asserts to Assumes":
(https://github.com/bitcoin/bitcoin/pull/28678#issuecomment-2413349588)
@sipa @darosior It would be good to mark this as up-for-grabs, if you dropped working on it.
(https://github.com/bitcoin/bitcoin/pull/28678#issuecomment-2413349588)
@sipa @darosior It would be good to mark this as up-for-grabs, if you dropped working on it.
💬 hebasto commented on pull request "build: Bump minimum supported macOS to 13.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413356842)
> > > Running Bitcoin Core on unsupported OSes may expose users to security issues.
> > > macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
> >
> >
> > If this is the reasoning we are going to use, shouldn't this be bumping to `10.13`, given `10.12` is also "unsupported" in terms of security updates?
>
> Yes, good point. I didn't realize 12.0 was eol'd. We shouldn't let qt dictate what we support.
>
> Agree that 13
...
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413356842)
> > > Running Bitcoin Core on unsupported OSes may expose users to security issues.
> > > macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
> >
> >
> > If this is the reasoning we are going to use, shouldn't this be bumping to `10.13`, given `10.12` is also "unsupported" in terms of security updates?
>
> Yes, good point. I didn't realize 12.0 was eol'd. We shouldn't let qt dictate what we support.
>
> Agree that 13
...
💬 maflcko commented on pull request "build: Bump minimum supported macOS to 13.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413362340)
lgtm ACK a0e089a71dc449f4cc177d6eb3050400e63f4b3f
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413362340)
lgtm ACK a0e089a71dc449f4cc177d6eb3050400e63f4b3f
💬 maflcko commented on issue "wallet: rpc: `settxfee` sets the wallet feerate not fee":
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2413394142)
In theory, it would be possible to register RPC argument name aliases, IIRC. They are denoted and split by `|`.
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2413394142)
In theory, it would be possible to register RPC argument name aliases, IIRC. They are denoted and split by `|`.
💬 m3dwards commented on pull request "build: Bump minimum supported macOS to 13.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413396346)
ACK a0e089a71dc449f4cc177d6eb3050400e63f4b3f
Tested on ARM Mac 14.6
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2413396346)
ACK a0e089a71dc449f4cc177d6eb3050400e63f4b3f
Tested on ARM Mac 14.6
⚠️ maflcko opened an issue: "doc: Fixup windows-cross build notes"
(https://github.com/bitcoin/bitcoin/issues/31090)
It is also missing bison and patch? Might as well remove it and just refer to the location that is correct?
_Originally posted by @maflcko in https://github.com/bitcoin/bitcoin/pull/31084#discussion_r1799629690_
(https://github.com/bitcoin/bitcoin/issues/31090)
It is also missing bison and patch? Might as well remove it and just refer to the location that is correct?
_Originally posted by @maflcko in https://github.com/bitcoin/bitcoin/pull/31084#discussion_r1799629690_
✅ stickies-v closed an issue: "test: C++ debugger interferes with functional tests that quickly stop/start nodes"
(https://github.com/bitcoin/bitcoin/issues/23651)
(https://github.com/bitcoin/bitcoin/issues/23651)
💬 stickies-v commented on issue "test: C++ debugger interferes with functional tests that quickly stop/start nodes":
(https://github.com/bitcoin/bitcoin/issues/23651#issuecomment-2413408647)
I'm going to close this issue, if anyone is interested in fixing this please feel free to do so. The issue still persists for me, but it's quite an edge case, I've not ran into anymore since reporting this almost three years ago.
I explored opening a PR for it using the below diff that forces removal of the `.lock` file, but it doesn't quite seem to fix things for me, and I don't feel like spending more time on it at the moment. I'd be happy to review a PR that addresses this, though.
<det
...
(https://github.com/bitcoin/bitcoin/issues/23651#issuecomment-2413408647)
I'm going to close this issue, if anyone is interested in fixing this please feel free to do so. The issue still persists for me, but it's quite an edge case, I've not ran into anymore since reporting this almost three years ago.
I explored opening a PR for it using the below diff that forces removal of the `.lock` file, but it doesn't quite seem to fix things for me, and I don't feel like spending more time on it at the moment. I'd be happy to review a PR that addresses this, though.
<det
...