💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1558917057)
> I'd rather we just not define this for MSVC.
Thanks! Done.
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1558917057)
> I'd rather we just not define this for MSVC.
Thanks! Done.
💬 instagibbs commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1":
(https://github.com/bitcoin/bitcoin/pull/28334#issuecomment-2046664715)
Leaning on -0 now for this PR.
Unfortunately I think mining pools may start hacking things off with their modifications, including flipping standardness switch in mainnet. From what I can tell, Marathon was doing exactly this feedback was given to introduce back some limits, such as upgrade hooks.
(https://github.com/bitcoin/bitcoin/pull/28334#issuecomment-2046664715)
Leaning on -0 now for this PR.
Unfortunately I think mining pools may start hacking things off with their modifications, including flipping standardness switch in mainnet. From what I can tell, Marathon was doing exactly this feedback was given to introduce back some limits, such as upgrade hooks.
💬 instagibbs commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#issuecomment-2046665936)
concept ack, will review once parent PR is merged
(https://github.com/bitcoin/bitcoin/pull/29827#issuecomment-2046665936)
concept ack, will review once parent PR is merged
⚠️ dergoegge opened an issue: "ci: Lower and unify default stack size"
(https://github.com/bitcoin/bitcoin/issues/29840)
Default stack size differs across operating systems. E.g. the defaults for thread stack sizes: Linux: 8MiB, Windows 1MiB, FreeBSD 2MiB, OpenBSD 512 KiB.
It would be sensible to lower the stack size used in all CI jobs to the lowest default, so we don't miss stack overflow bugs that only occur with small stack sizes.
(https://github.com/bitcoin/bitcoin/issues/29840)
Default stack size differs across operating systems. E.g. the defaults for thread stack sizes: Linux: 8MiB, Windows 1MiB, FreeBSD 2MiB, OpenBSD 512 KiB.
It would be sensible to lower the stack size used in all CI jobs to the lowest default, so we don't miss stack overflow bugs that only occur with small stack sizes.
👍 TheCharlatan approved a pull request: "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-1990944993)
Re-ACK ffff3d2bae599d872bae184c4d7c2b0dacfc6e26
(https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-1990944993)
Re-ACK ffff3d2bae599d872bae184c4d7c2b0dacfc6e26
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1558971541)
```
net_processing.cpp:3224:40: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
3224 | for (auto i{package.size() - 1}; i >= 0; --i) {
|
```
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1558971541)
```
net_processing.cpp:3224:40: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
3224 | for (auto i{package.size() - 1}; i >= 0; --i) {
|
```
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1558979841)
this is also causing the sanitizer run failure
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1558979841)
this is also causing the sanitizer run failure
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1557310808)
```suggestion
* The senders arg should be populated in same order as individual transactions
* in the package_result argument.
*/
```
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1557310808)
```suggestion
* The senders arg should be populated in same order as individual transactions
* in the package_result argument.
*/
```
💬 maflcko commented on issue "ci: Lower and unify default stack size":
(https://github.com/bitcoin/bitcoin/issues/29840#issuecomment-2046737358)
Any reason to not use half of the minimum, to more likely catch edge cases that would happen on unknown operating systems, or to more likely catch a runtime edge-case that is hit in production and not in the tests?
(https://github.com/bitcoin/bitcoin/issues/29840#issuecomment-2046737358)
Any reason to not use half of the minimum, to more likely catch edge cases that would happen on unknown operating systems, or to more likely catch a runtime edge-case that is hit in production and not in the tests?
✅ dergoegge closed an issue: "Disallow certain message types under BIP324 v2 transport"
(https://github.com/bitcoin/bitcoin/issues/29632)
(https://github.com/bitcoin/bitcoin/issues/29632)
📝 fanquake opened a pull request: "fuzz: use std::source_location in fuzz/rpc.cpp"
(https://github.com/bitcoin/bitcoin/pull/29841)
Introduce a usage of [`std::source_location`](https://en.cppreference.com/w/cpp/utility/source_location).
I thought this had been/was blocked on the macOS SDK.
(https://github.com/bitcoin/bitcoin/pull/29841)
Introduce a usage of [`std::source_location`](https://en.cppreference.com/w/cpp/utility/source_location).
I thought this had been/was blocked on the macOS SDK.
💬 maflcko commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046766022)
-0 on this refactor. The existing code seems easier to read and is shorter?
Also, albeit not an issue here, when used as default argument the location is wrong, see https://github.com/llvm/llvm-project/issues/56379.
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046766022)
-0 on this refactor. The existing code seems easier to read and is shorter?
Also, albeit not an issue here, when used as default argument the location is wrong, see https://github.com/llvm/llvm-project/issues/56379.
💬 fanquake commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046772255)
> The existing code seems easier to read and is shorter?
Yea, I assumed this might be one pushback. I guess the project can decide if this is actually something we want to use in general. Or, if the tradeoff in verbosity is only worth it when used more globally.
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046772255)
> The existing code seems easier to read and is shorter?
Yea, I assumed this might be one pushback. I guess the project can decide if this is actually something we want to use in general. Or, if the tradeoff in verbosity is only worth it when used more globally.
✅ fanquake closed a pull request: "fuzz: use std::source_location in fuzz/rpc.cpp"
(https://github.com/bitcoin/bitcoin/pull/29841)
(https://github.com/bitcoin/bitcoin/pull/29841)
💬 fanquake commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046775039)
Actually, will just come back to this after Clang 16.
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046775039)
Actually, will just come back to this after Clang 16.
💬 maflcko commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046784120)
The only reasonable places to use this would be where it is used as a default argument, no? So that'd be:
* https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1850520824
* https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445230410
Ref #29077 for the fix.
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046784120)
The only reasonable places to use this would be where it is used as a default argument, no? So that'd be:
* https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1850520824
* https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445230410
Ref #29077 for the fix.
📝 Thales-de-Milet opened a pull request: "Create docker-image.yml"
(https://github.com/bitcoin-core/gui/pull/816)
<!--
*** seed:0708ce02816c430ab90bebbf40828693f7f5d111ba0f2b169e5b83213f0200257bd3baa4a2a47b7d115db23aa6de960d48796a3bb1565735a6d349e92abdca4e ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/Thales-de-Miles/Bitcoin/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin C
...
(https://github.com/bitcoin-core/gui/pull/816)
<!--
*** seed:0708ce02816c430ab90bebbf40828693f7f5d111ba0f2b169e5b83213f0200257bd3baa4a2a47b7d115db23aa6de960d48796a3bb1565735a6d349e92abdca4e ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/Thales-de-Miles/Bitcoin/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin C
...
✅ fanquake closed a pull request: "Create docker-image.yml"
(https://github.com/bitcoin-core/gui/pull/816)
(https://github.com/bitcoin-core/gui/pull/816)
💬 0xB10C commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2046870296)
I've been playing around with bencher running the `bitcoin_bench` bitcoind binary in a GH action as PoC. A sample dashboard is [here](https://bencher.dev/console/projects/0xb10c-s-bitcoin-core/perf?key=true&reports_per_page=4&branches_per_page=8&testbeds_per_page=8&benchmarks_per_page=8&reports_page=1&branches_page=1&testbeds_page=1&benchmarks_page=1&report=915c1007-33dd-4d30-b9c1-531b60af8a06&branches=0ab6caf7-716b-4a6e-8101-f2b9c891a9a2&testbeds=3c0c4d9d-b40e-4d9a-a462-2379355b0785&benchmarks=
...
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2046870296)
I've been playing around with bencher running the `bitcoin_bench` bitcoind binary in a GH action as PoC. A sample dashboard is [here](https://bencher.dev/console/projects/0xb10c-s-bitcoin-core/perf?key=true&reports_per_page=4&branches_per_page=8&testbeds_per_page=8&benchmarks_per_page=8&reports_page=1&branches_page=1&testbeds_page=1&benchmarks_page=1&report=915c1007-33dd-4d30-b9c1-531b60af8a06&branches=0ab6caf7-716b-4a6e-8101-f2b9c891a9a2&testbeds=3c0c4d9d-b40e-4d9a-a462-2379355b0785&benchmarks=
...
💬 real-or-random commented on pull request "Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305":
(https://github.com/bitcoin/bitcoin/pull/28263#issuecomment-2046884446)
@theStack want to re-review this? :)
(https://github.com/bitcoin/bitcoin/pull/28263#issuecomment-2046884446)
@theStack want to re-review this? :)