📝 glozow opened a pull request: "package validation: relax the package-not-child-with-unconfirmed-parents rule"
(https://github.com/bitcoin/bitcoin/pull/31385)
Draft because builds on #31096
On master, package rules include (1) topology needs to be child-wth-parents (2) all of the child's unconfirmed parents need to be present. This PR relaxes the second rule. The first rule is untouched.
Original motivation for this rule was based on the idea that we would have a child-with-unconfirmed-parents package relay protocol, and this would verify that the peer provided the "correct" package. For various reasons, we're not planning on doing this. We coul
...
(https://github.com/bitcoin/bitcoin/pull/31385)
Draft because builds on #31096
On master, package rules include (1) topology needs to be child-wth-parents (2) all of the child's unconfirmed parents need to be present. This PR relaxes the second rule. The first rule is untouched.
Original motivation for this rule was based on the idea that we would have a child-with-unconfirmed-parents package relay protocol, and this would verify that the peer provided the "correct" package. For various reasons, we're not planning on doing this. We coul
...
👍 tdb3 approved a pull request: "doc, test: more ephemeral dust follow-ups"
(https://github.com/bitcoin/bitcoin/pull/31371#pullrequestreview-2466174195)
Code review ACK 160799d9135528dbdea40690f0bb0d56c6c4803a
Noticed that `mempool_ephemeral_dust.py` takes a significant amount of time to run (around 60s on at least on two of my machines), but is near the bottom of `BASE_SCRIPTS` in `test_runner.py`. Moving the test toward the front of the list significantly improved parallel test runtime on each of my machines (e.g. from 131s to 91s). ymmv. I'm happy to submit a separate PR to prevent ACK invalidation.
```diff
diff --git a/test/functional
...
(https://github.com/bitcoin/bitcoin/pull/31371#pullrequestreview-2466174195)
Code review ACK 160799d9135528dbdea40690f0bb0d56c6c4803a
Noticed that `mempool_ephemeral_dust.py` takes a significant amount of time to run (around 60s on at least on two of my machines), but is near the bottom of `BASE_SCRIPTS` in `test_runner.py`. Moving the test toward the front of the list significantly improved parallel test runtime on each of my machines (e.g. from 131s to 91s). ymmv. I'm happy to submit a separate PR to prevent ACK invalidation.
```diff
diff --git a/test/functional
...
📝 JeremyRand opened a pull request: "doc: Use more precise anchor links to Xcode SDK extraction"
(https://github.com/bitcoin/bitcoin/pull/31386)
The "SDK Extraction" section is what users presumably are looking for when they follow these links.
(https://github.com/bitcoin/bitcoin/pull/31386)
The "SDK Extraction" section is what users presumably are looking for when they follow these links.
📝 JeremyRand opened a pull request: "doc: Use more precise anchor link to codesigning docs"
(https://github.com/bitcoin/bitcoin/pull/31387)
The "Codesigning" section is what users presumably are looking for when they follow this link.
(https://github.com/bitcoin/bitcoin/pull/31387)
The "Codesigning" section is what users presumably are looking for when they follow this link.
💬 maflcko commented on pull request "test: Avoid intermittent block download timeout in p2p_ibd_stalling":
(https://github.com/bitcoin/bitcoin/pull/30705#issuecomment-2505414701)
Fixup in https://github.com/bitcoin/bitcoin/pull/31383
(https://github.com/bitcoin/bitcoin/pull/30705#issuecomment-2505414701)
Fixup in https://github.com/bitcoin/bitcoin/pull/31383
💬 rkrux commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1861638355)
Ah I see, makes sense to me.
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1861638355)
Ah I see, makes sense to me.
💬 rkrux commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1861656216)
Oh interesting, thanks for the clarification. It was seeing `fee_per_output=dust_tx_fee` together that threw me off. Maybe an argument comment here at this call site of `create self transfer multi` can be more clarifying stating that there is only 1 output created initially? That is if the file is retouched.
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1861656216)
Oh interesting, thanks for the clarification. It was seeing `fee_per_output=dust_tx_fee` together that threw me off. Maybe an argument comment here at this call site of `create self transfer multi` can be more clarifying stating that there is only 1 output created initially? That is if the file is retouched.
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1861672332)
I don't think this is sufficient. The CI failure (https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2499335672) will remain, depending on a vendor default.
Again, my preference would be to explicitly list the required (or removed) caps, instead of relying on a vendor default. Otherwise, it will become harder to run the CI locally, or lead to vendor-lock-in.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1861672332)
I don't think this is sufficient. The CI failure (https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2499335672) will remain, depending on a vendor default.
Again, my preference would be to explicitly list the required (or removed) caps, instead of relying on a vendor default. Otherwise, it will become harder to run the CI locally, or lead to vendor-lock-in.
💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1861679662)
> Looking at the actual usage, `ExpectSuccess` doesn't need to be a variadic template function:
This may or may not be true right now, but the function has been intentionally written the way it is now. This is a pretty standard and common boilerplate code to pass a variable number of args to a constructor.
Changing it would be problematic, because it will need to be touched again in the future to be reverted, if there is a constructor with more than one arg.
Also, your suggestion will f
...
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1861679662)
> Looking at the actual usage, `ExpectSuccess` doesn't need to be a variadic template function:
This may or may not be true right now, but the function has been intentionally written the way it is now. This is a pretty standard and common boilerplate code to pass a variable number of args to a constructor.
Changing it would be problematic, because it will need to be touched again in the future to be reverted, if there is a constructor with more than one arg.
Also, your suggestion will f
...
💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2505569501)
> maybe time to introduce a replace mempool method like Carl suggested there?
Did this now, ready for review.
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2505569501)
> maybe time to introduce a replace mempool method like Carl suggested there?
Did this now, ready for review.
👋 TheCharlatan's pull request is ready for review: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382)
(https://github.com/bitcoin/bitcoin/pull/31382)
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2505597125)
I rebased https://github.com/Sjors/bitcoin/pull/48 to use the latest version of this PR, to figure out what I need for cmake. The only thing I still suggest is:
* add `bitcoin-mine` to the `foreach(target IN ITEMS` list in `cmake/module/Maintenance.cmake`. IIUC that will cause "the Guix script to run symbol-check.py and security-check.py on them". Even though this PR doesn't add bitcoin-mine to the guix build artefacts, it seems better to add it to this list now so we don't forget.
ACK 270
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2505597125)
I rebased https://github.com/Sjors/bitcoin/pull/48 to use the latest version of this PR, to figure out what I need for cmake. The only thing I still suggest is:
* add `bitcoin-mine` to the `foreach(target IN ITEMS` list in `cmake/module/Maintenance.cmake`. IIUC that will cause "the Guix script to run symbol-check.py and security-check.py on them". Even though this PR doesn't add bitcoin-mine to the guix build artefacts, it seems better to add it to this list now so we don't forget.
ACK 270
...
💬 fanquake commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2505607490)
> it seems better to add it to this list now so we don't forget.
Nack. We can add things to Guix and the release process when they are actually part of a release.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2505607490)
> it seems better to add it to this list now so we don't forget.
Nack. We can add things to Guix and the release process when they are actually part of a release.
🤔 Sjors reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2467341532)
Concept ACK
I think it would be more clear to move `build/src/bitcoin-{node,gui}` to `build/src/libexec`, rather than use a different file organization for CMake builds than for installs.
The "Win64 native, VS 2022" job still seems unhappy.
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2467341532)
Concept ACK
I think it would be more clear to move `build/src/bitcoin-{node,gui}` to `build/src/libexec`, rather than use a different file organization for CMake builds than for installs.
The "Win64 native, VS 2022" job still seems unhappy.
💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807)
b06c7ad0ae91102fe8cdcac6f86d627aace2219b: this is potentially confusing. If I build without gui, I expect `build/src/bitcoin gui` to fail. Right now it would launch any `bitcoin-qt` in my `$PATH`.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807)
b06c7ad0ae91102fe8cdcac6f86d627aace2219b: this is potentially confusing. If I build without gui, I expect `build/src/bitcoin gui` to fail. Right now it would launch any `bitcoin-qt` in my `$PATH`.
⚠️ Sjors opened an issue: "ARM Windows build"
(https://github.com/bitcoin/bitcoin/issues/31388)
This was brought up recently:
* https://bitcointalk.org/index.php?topic=5517601.0
* https://groups.google.com/d/msgid/bitcoindev/e5b06aaa-1fe9-4c8f-a0ea-db10f8a7e48cn%40googlegroups.com
(https://github.com/bitcoin/bitcoin/issues/31388)
This was brought up recently:
* https://bitcointalk.org/index.php?topic=5517601.0
* https://groups.google.com/d/msgid/bitcoindev/e5b06aaa-1fe9-4c8f-a0ea-db10f8a7e48cn%40googlegroups.com
💬 Sjors commented on issue "ARM Windows build":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-2505742737)
I believe @ZenulAbidin asked on the Bitcoin Dev list?
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-2505742737)
I believe @ZenulAbidin asked on the Bitcoin Dev list?
💬 maflcko commented on issue "ARM Windows build":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-2505815571)
I am not sure how easy it would be to ship windows arm release binaries in guix, or if it is even possible today. I guess not, based on https://github.com/Windows-on-ARM-Experiments/mingw-woarm64-build?tab=readme-ov-file#known-issues .
As for the MSVC build, someone would have to go through https://github.com/bitcoin/bitcoin/blob/master/doc/build-windows-msvc.md with a recent ARM64 MSVC, see https://learn.microsoft.com/en-us/visualstudio/install/visual-studio-on-arm-devices?view=vs-2022#get-s
...
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-2505815571)
I am not sure how easy it would be to ship windows arm release binaries in guix, or if it is even possible today. I guess not, based on https://github.com/Windows-on-ARM-Experiments/mingw-woarm64-build?tab=readme-ov-file#known-issues .
As for the MSVC build, someone would have to go through https://github.com/bitcoin/bitcoin/blob/master/doc/build-windows-msvc.md with a recent ARM64 MSVC, see https://learn.microsoft.com/en-us/visualstudio/install/visual-studio-on-arm-devices?view=vs-2022#get-s
...
💬 hebasto commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2505819725)
> The "Win64 native, VS 2022" job still seems unhappy.
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996?view=msvc-170#posix-function-names:
> To turn off deprecation warnings for these functions, define the preprocessor macro `_CRT_NONSTDC_NO_WARNINGS`. You can define this macro at the command line by including the option `/D_CRT_NONSTDC_NO_WARNINGS`.
We have already used this macro:https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3b
...
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2505819725)
> The "Win64 native, VS 2022" job still seems unhappy.
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996?view=msvc-170#posix-function-names:
> To turn off deprecation warnings for these functions, define the preprocessor macro `_CRT_NONSTDC_NO_WARNINGS`. You can define this macro at the command line by including the option `/D_CRT_NONSTDC_NO_WARNINGS`.
We have already used this macro:https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3b
...
💬 theStack commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#issuecomment-2505864742)
> Noticed that `mempool_ephemeral_dust.py` takes a significant amount of time to run (around 50s on at least on two of my machines, i.e. greater than 30s), but is near the bottom of `BASE_SCRIPTS` in `test_runner.py`. Moving the test toward the front of the list significantly improved parallel test runtime on each of my machines (e.g. from 131s to 91s, jobs=32). ymmv. I'm happy to submit a separate PR ([tdb3@3c40aac](https://github.com/tdb3/bitcoin/commit/3c40aac84972e0887ecd41161231a08f87d1efa3
...
(https://github.com/bitcoin/bitcoin/pull/31371#issuecomment-2505864742)
> Noticed that `mempool_ephemeral_dust.py` takes a significant amount of time to run (around 50s on at least on two of my machines, i.e. greater than 30s), but is near the bottom of `BASE_SCRIPTS` in `test_runner.py`. Moving the test toward the front of the list significantly improved parallel test runtime on each of my machines (e.g. from 131s to 91s, jobs=32). ymmv. I'm happy to submit a separate PR ([tdb3@3c40aac](https://github.com/tdb3/bitcoin/commit/3c40aac84972e0887ecd41161231a08f87d1efa3
...