💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2447224880)
re: https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2447182783
> mismatches between our custom `consteval` checking and tinyformat are gone as far as our current tests go
In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat
- rebased commit: [32d4cf3](https://github.com/bitcoin/bitcoin/commit/32d4cf37d5056dc42bbf317b059e10910b984b0e)
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2447224880)
re: https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2447182783
> mismatches between our custom `consteval` checking and tinyformat are gone as far as our current tests go
In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat
- rebased commit: [32d4cf3](https://github.com/bitcoin/bitcoin/commit/32d4cf37d5056dc42bbf317b059e10910b984b0e)
💬 maflcko commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2447229422)
(force pushed to fix a type bug in the modified Python code, discovered by the Windows CI. I wish this code was written in Rust or another type-safe language, so that issues like this are caught at compile-time, but this can be done in a follow-up)
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2447229422)
(force pushed to fix a type bug in the modified Python code, discovered by the Windows CI. I wish this code was written in Rust or another type-safe language, so that issues like this are caught at compile-time, but this can be done in a follow-up)
✅ maflcko closed a pull request: "doc: note that you can assume C++20."
(https://github.com/bitcoin/bitcoin/pull/30136)
(https://github.com/bitcoin/bitcoin/pull/30136)
💬 maflcko commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2447301242)
Closing for now, due to inactivity since May. (The feedback was waiting to be addressed since then)
Please leave a comment if you want this reopened, or you can open a new pull request with the new changes.
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2447301242)
Closing for now, due to inactivity since May. (The feedback was waiting to be addressed since then)
Please leave a comment if you want this reopened, or you can open a new pull request with the new changes.
💬 davidgumberg commented on pull request "key: clear out secret data in `DecodeExtKey`":
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2447431280)
utACK https://github.com/bitcoin/bitcoin/pull/31166/commits/559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c
I agree that given how short lived this data is it doesn't seem likely to get paged, but it would still be nice to encapsulate our allocation strategy for secrets.
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2447431280)
utACK https://github.com/bitcoin/bitcoin/pull/31166/commits/559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c
I agree that given how short lived this data is it doesn't seem likely to get paged, but it would still be nice to encapsulate our allocation strategy for secrets.
💬 theStack commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2447431668)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2447431668)
Concept ACK
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2447434066)
Rebasing to deal with merge conflicts
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2447434066)
Rebasing to deal with merge conflicts
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1822834821)
I think the current approach mimics what you were doing with your old one. I haven't picked up the trickle reduction commit not the delayed response, but I was planning to in the next PR.
Your approach used to add transactions to the reconciliation set during INV building, which happens on trickle intervals. So data was added to `to_be_announced` (delayed) on `RelayTransaction` and then made available on trickle. The current approach adds data to the delayed set on `RelayTransaction` and make
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1822834821)
I think the current approach mimics what you were doing with your old one. I haven't picked up the trickle reduction commit not the delayed response, but I was planning to in the next PR.
Your approach used to add transactions to the reconciliation set during INV building, which happens on trickle intervals. So data was added to `to_be_announced` (delayed) on `RelayTransaction` and then made available on trickle. The current approach adds data to the delayed set on `RelayTransaction` and make
...
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2447569710)
Added the doc commit, ready for further review
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2447569710)
Added the doc commit, ready for further review
💬 jb55 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2447587199)
> In my main machine, is ryzen with 32gb ddr4 with 2 sticks of memory. The other machine is Xeon, with 32gb memory, in quad channel ddr3. So i dont think its memory, because the 2 computers are stable. I can run prime95 in both for 2 hours with no problem.
it's true, I just ran disk diagnostics, no issues. going to run a memory diagnostic... but my system has been stable so I would be very surprised if it was that. I'm also running on ZFS. I keep hitting this, even after a full redownload...
...
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2447587199)
> In my main machine, is ryzen with 32gb ddr4 with 2 sticks of memory. The other machine is Xeon, with 32gb memory, in quad channel ddr3. So i dont think its memory, because the 2 computers are stable. I can run prime95 in both for 2 hours with no problem.
it's true, I just ran disk diagnostics, no issues. going to run a memory diagnostic... but my system has been stable so I would be very surprised if it was that. I'm also running on ZFS. I keep hitting this, even after a full redownload...
...
💬 maflcko commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1822897369)
Hex parsing is used in mining via DecodeHexBlk, no?
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1822897369)
Hex parsing is used in mining via DecodeHexBlk, no?
💬 dergoegge commented on issue "Support validating a PoW-free block over over RPC":
(https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2447653274)
At a glance, [this](https://github.com/bitcoin/bitcoin/blob/97b790e844abd2f92c928239a7dc786d37fad18b/src/rpc/mining.cpp#L683-L710) looks like what you're looking for? i.e. `getblocktemplate`'s proposal mode: https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki#block-proposal
(https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2447653274)
At a glance, [this](https://github.com/bitcoin/bitcoin/blob/97b790e844abd2f92c928239a7dc786d37fad18b/src/rpc/mining.cpp#L683-L710) looks like what you're looking for? i.e. `getblocktemplate`'s proposal mode: https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki#block-proposal
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1822955914)
Does that meaningfully impact block submission? if so, seems like `DecodeHexBlk` should be the target for the benchmark.
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1822955914)
Does that meaningfully impact block submission? if so, seems like `DecodeHexBlk` should be the target for the benchmark.
💬 sr-gi commented on issue "`Wunused-member-function` in test each commit":
(https://github.com/bitcoin/bitcoin/issues/31180#issuecomment-2447895711)
> It may be trivial to suppress either way by using https://en.cppreference.com/w/cpp/language/attributes/maybe_unused in the commit?
>
> It should also be trivial to remove the error completely from the CI task by specifying `Wno-error=unused-member-function`?
Agreed on both. The purpose of the issue was mostly to agree on what the expectations are for methods that may be used within the same PR but not within the commit they were defined in.
Will open a PR adding `Wno-error=unused-mem
...
(https://github.com/bitcoin/bitcoin/issues/31180#issuecomment-2447895711)
> It may be trivial to suppress either way by using https://en.cppreference.com/w/cpp/language/attributes/maybe_unused in the commit?
>
> It should also be trivial to remove the error completely from the CI task by specifying `Wno-error=unused-member-function`?
Agreed on both. The purpose of the issue was mostly to agree on what the expectations are for methods that may be used within the same PR but not within the commit they were defined in.
Will open a PR adding `Wno-error=unused-mem
...
📝 hebasto opened a pull request: "msvc: Update vcpkg manifest"
(https://github.com/bitcoin/bitcoin/pull/31186)
This PR updates the vcpkg manifest baseline from the [2023.08.09 Release ](https://github.com/microsoft/vcpkg/releases/tag/2023.08.09) to the [2024.09.30 Release](https://github.com/microsoft/vcpkg/releases/tag/2024.09.30), with the following package changes:
- `boost`: 1.82.0#2 --> 1.85.0#1,2
- `qt5`: 5.15.10#5 -> 5.15.15
- `sqlite3`: 3.42.0#1 --> 3.46.1
- `zeromq`: 2023-06-20#1 --> 4.3.5#2
The previous update was made in https://github.com/bitcoin/bitcoin/pull/28938.
For additional m
...
(https://github.com/bitcoin/bitcoin/pull/31186)
This PR updates the vcpkg manifest baseline from the [2023.08.09 Release ](https://github.com/microsoft/vcpkg/releases/tag/2023.08.09) to the [2024.09.30 Release](https://github.com/microsoft/vcpkg/releases/tag/2024.09.30), with the following package changes:
- `boost`: 1.82.0#2 --> 1.85.0#1,2
- `qt5`: 5.15.10#5 -> 5.15.15
- `sqlite3`: 3.42.0#1 --> 3.46.1
- `zeromq`: 2023-06-20#1 --> 4.3.5#2
The previous update was made in https://github.com/bitcoin/bitcoin/pull/28938.
For additional m
...
📝 sr-gi opened a pull request: "ci: Do not error on unused-member-function in test each commit"
(https://github.com/bitcoin/bitcoin/pull/31187)
After https://github.com/bitcoin/bitcoin/pull/31045, an unused method in a commit will trigger a compilation error, even if that method is later used in a following commit within the same PR.
Do not enforce unused-member-function in test each commit.
Close #31180
(https://github.com/bitcoin/bitcoin/pull/31187)
After https://github.com/bitcoin/bitcoin/pull/31045, an unused method in a commit will trigger a compilation error, even if that method is later used in a following commit within the same PR.
Do not enforce unused-member-function in test each commit.
Close #31180
💬 davidgumberg commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2447990044)
Concept ACK, Approach NACK
I think there should be a burden on PR's that claim to "improve performance" to demonstrate a user-impacting scenario where the improvements are relevant, and agree that having benchmarks that test workloads that will never impact users leads to PR's being opened and review being spent on things that don't deserve attention.
But, I think a similar burden exists for this PR to demonstrate or at least describe a rationale for why these removed benchmarks don't test
...
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2447990044)
Concept ACK, Approach NACK
I think there should be a burden on PR's that claim to "improve performance" to demonstrate a user-impacting scenario where the improvements are relevant, and agree that having benchmarks that test workloads that will never impact users leads to PR's being opened and review being spent on things that don't deserve attention.
But, I think a similar burden exists for this PR to demonstrate or at least describe a rationale for why these removed benchmarks don't test
...
💬 achow101 commented on pull request "key: clear out secret data in `DecodeExtKey`":
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2447994081)
ACK 559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2447994081)
ACK 559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c
💬 edilmedeiros commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2448004215)
Looks like this is not relevant anyway after merging the cmake build.
Lack of this info in the docs doesn't seem to be an issue.
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2448004215)
Looks like this is not relevant anyway after merging the cmake build.
Lack of this info in the docs doesn't seem to be an issue.
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1823207236)
You're right, should have been added to the previous commit. Will fix
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1823207236)
You're right, should have been added to the previous commit. Will fix