💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867942188)
nits:
Although there is precedent for adding references to gists, I'm not sure we should encourage it. Would have preferred a file in this repo's *contrib/* directory.
Would also have preferred that it did the full calculation of the .tgz file instead of having a hard-coded array, computing it by looking at linearalized blocks on disk.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867942188)
nits:
Although there is precedent for adding references to gists, I'm not sure we should encourage it. Would have preferred a file in this repo's *contrib/* directory.
Would also have preferred that it did the full calculation of the .tgz file instead of having a hard-coded array, computing it by looking at linearalized blocks on disk.
💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1868020679)
9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24 before the latest push didn't have these `const` params. I think it was a mistake to add them in the latest push and would prefer them rolled back.
Another alternative is to discuss my samples of ratios of `const` value type params or why you didn't apply it to `AddFlags`.
Sorry to be annoying about this but for some reason it really annoys me. :)
Will take a timeout and see if I can let go of this, if you still insist.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1868020679)
9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24 before the latest push didn't have these `const` params. I think it was a mistake to add them in the latest push and would prefer them rolled back.
Another alternative is to discuss my samples of ratios of `const` value type params or why you didn't apply it to `AddFlags`.
Sorry to be annoying about this but for some reason it really annoys me. :)
Will take a timeout and see if I can let go of this, if you still insist.
📝 instagibbs opened a pull request: "func: test orphan parent is re-requested from 2nd peer"
(https://github.com/bitcoin/bitcoin/pull/31414)
Small test which I couldn't find coverage for, and with the current test being a bit misleading in comments.
(https://github.com/bitcoin/bitcoin/pull/31414)
Small test which I couldn't find coverage for, and with the current test being a bit misleading in comments.
💬 darosior commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#discussion_r1868051679)
Because it is unnecessary?
(https://github.com/bitcoin/bitcoin/pull/31376#discussion_r1868051679)
Because it is unnecessary?
💬 achow101 commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515086614)
> I’m not sure how what you are explaining is in way different than the normal expected behavior? Code signing is done by the developer, and the notarization is done is done by Apple. I’ve been doing this third-party submission to Apple for a while now, so I can make stapled bitcoin binaries for my own use.
How is that the normal expected behavior? I've been through the as many docs on notarization as I could find and I don't think there was any mention of third parties being able to submit a
...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515086614)
> I’m not sure how what you are explaining is in way different than the normal expected behavior? Code signing is done by the developer, and the notarization is done is done by Apple. I’ve been doing this third-party submission to Apple for a while now, so I can make stapled bitcoin binaries for my own use.
How is that the normal expected behavior? I've been through the as many docs on notarization as I could find and I don't think there was any mention of third parties being able to submit a
...
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1868079220)
The codesigners are not necessarily maintainers. I don't think it's useful to make this broad when it has been extremely specific for codesigning for more than a decade.
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1868079220)
The codesigners are not necessarily maintainers. I don't think it's useful to make this broad when it has been extremely specific for codesigning for more than a decade.
💬 ryanofsky commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515108628)
Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic. I wonder if you think there are concrete downsides to this PR, or if the changes are ok but possibly not be worth the review effort (as https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449824362 seems to suggest)
I'm happy to spend time reviewing this if it improves performance and doesn't cause other problems.
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515108628)
Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic. I wonder if you think there are concrete downsides to this PR, or if the changes are ok but possibly not be worth the review effort (as https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449824362 seems to suggest)
I'm happy to spend time reviewing this if it improves performance and doesn't cause other problems.
...
📝 BrandonOdiwuor opened a pull request: "test: fix TestShell initialization and reset()"
(https://github.com/bitcoin/bitcoin/pull/31415)
Fixes TestShell initialization issues caused by resolving symlinks and looking for config.ini in the source path instead of the build path after migration to CMake (see https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2433056070)
https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/test/functional/test_framework/test_shell.py#L77
also fixes https://github.com/bitcoin/bitcoin/issues/31131
(https://github.com/bitcoin/bitcoin/pull/31415)
Fixes TestShell initialization issues caused by resolving symlinks and looking for config.ini in the source path instead of the build path after migration to CMake (see https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2433056070)
https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/test/functional/test_framework/test_shell.py#L77
also fixes https://github.com/bitcoin/bitcoin/issues/31131
💬 maflcko commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515129785)
> Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic.
There is a good chance that increasing the size of the vector is insufficient, if there is ever a need to increase it to more than 8 bytes, so a complete rewrite may be needed in that case anyway. However, this is just my guess and only time will tell. So I'd say this change is probably fine for now.
Would still be nic
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515129785)
> Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic.
There is a good chance that increasing the size of the vector is insufficient, if there is ever a need to increase it to more than 8 bytes, so a complete rewrite may be needed in that case anyway. However, this is just my guess and only time will tell. So I'd say this change is probably fine for now.
Would still be nic
...
💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515135185)
@ryanofsky Do you think this is rfm?
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515135185)
@ryanofsky Do you think this is rfm?
💬 ryanofsky commented on pull request "rpc: Remove deprecated RPC arg names":
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2515136809)
re: https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514938972
> -rpcdeprecated is limited both spatially and temporary: To only the RPC method that needs it, not the whole RPC framework, and only to a single release. So it seems preferable to me.
Got it, so it sounds like a big motivation is to simplify rpc framework code. Agree that's worth doing, and I think that should be possible even with #24963, or we could revisit this approach if #24963 doesn't go anywhere.
> re: https
...
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2515136809)
re: https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514938972
> -rpcdeprecated is limited both spatially and temporary: To only the RPC method that needs it, not the whole RPC framework, and only to a single release. So it seems preferable to me.
Got it, so it sounds like a big motivation is to simplify rpc framework code. Agree that's worth doing, and I think that should be possible even with #24963, or we could revisit this approach if #24963 doesn't go anywhere.
> re: https
...
🚀 ryanofsky merged a pull request: "ci: Skip broken Wine64 tests by default"
(https://github.com/bitcoin/bitcoin/pull/31284)
(https://github.com/bitcoin/bitcoin/pull/31284)
👍 lucasbalieiro approved a pull request: "test: fix `test_invalid_tx_in_compactblock` in `p2p_compactblocks`"
(https://github.com/bitcoin/bitcoin/pull/31406#pullrequestreview-2476348912)
Tested ACK: [https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65](https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65)
Tested in my environment:
```bash
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: noble
```
Everything ran smoothly. I understand the necessity of the change to ensure correct comparisons between integer values.
For future
...
(https://github.com/bitcoin/bitcoin/pull/31406#pullrequestreview-2476348912)
Tested ACK: [https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65](https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65)
Tested in my environment:
```bash
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: noble
```
Everything ran smoothly. I understand the necessity of the change to ensure correct comparisons between integer values.
For future
...
💬 ryanofsky commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515177947)
> @ryanofsky Do you think this is rfm?
Merged this now.
Am a little curious about the underlying issue though because in theory there doesn't have to be binary distinction between false positives and true positives. Test failures on a platform could indicate that be a sign that tests are fragile or poorly written even if they aren't technically buggy or reveal bugs in the code. Just clicking the cirrus links here it's hard to tell what the nature of these failures is, what may be causing t
...
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515177947)
> @ryanofsky Do you think this is rfm?
Merged this now.
Am a little curious about the underlying issue though because in theory there doesn't have to be binary distinction between false positives and true positives. Test failures on a platform could indicate that be a sign that tests are fragile or poorly written even if they aren't technically buggy or reveal bugs in the code. Just clicking the cirrus links here it's hard to tell what the nature of these failures is, what may be causing t
...
🤔 mzumsande reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2476392187)
Code Review ACK 492e1f09943fcb6145c21d470299305a19e17d8b
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2476392187)
Code Review ACK 492e1f09943fcb6145c21d470299305a19e17d8b
💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515212570)
> Would still be nice to if there was a way to take all of it out of the hot path
Can you give me hints on how to do that?
Since we have a primitive as a key now, we already skip xor with 0 value now, see https://github.com/bitcoin/bitcoin/pull/31144/files#diff-4020c723bb55e114bdc7ff769086a765dcc7ccfb61da2047a315db16c0c7a8b4R295
> but wondering it it might be better to just use a sampling of the most common write sizes
@fanquake mentioned that he thinks this benchmark could be useful -
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515212570)
> Would still be nice to if there was a way to take all of it out of the hot path
Can you give me hints on how to do that?
Since we have a primitive as a key now, we already skip xor with 0 value now, see https://github.com/bitcoin/bitcoin/pull/31144/files#diff-4020c723bb55e114bdc7ff769086a765dcc7ccfb61da2047a315db16c0c7a8b4R295
> but wondering it it might be better to just use a sampling of the most common write sizes
@fanquake mentioned that he thinks this benchmark could be useful -
...
💬 maaku commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515230237)
What I’m saying is: what difference does it make who submits it? Given the security model, it would be weird to gatekeep (hehe) it to the original developer. It’s just asking Apple to run some checks on the binary, that’s all.
It makes no difference at all if the submitter is the same as the signer. Lots of people take advantage of this workflow, so it isn’t a hidden gotcha.
Notarization isn’t code signing. The submission identity check is merely a DoS prevention mechanism for Apple’s gate
...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515230237)
What I’m saying is: what difference does it make who submits it? Given the security model, it would be weird to gatekeep (hehe) it to the original developer. It’s just asking Apple to run some checks on the binary, that’s all.
It makes no difference at all if the submitter is the same as the signer. Lots of people take advantage of this workflow, so it isn’t a hidden gotcha.
Notarization isn’t code signing. The submission identity check is merely a DoS prevention mechanism for Apple’s gate
...
💬 achow101 commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515240900)
> What I’m saying is: what difference does it make who submits it? Given the security model, it would be weird to gatekeep it (hehe) to the original developer. It’s just asking Apple to run some checks on the binary, that’s all.
There's no different; it doesn't matter.
That statement is not a complaint about notarization nor am I saying it is a reason to not notarize. It is simply an observation that I found surprising. If that were common knowledge, I would have assumed someone would have
...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515240900)
> What I’m saying is: what difference does it make who submits it? Given the security model, it would be weird to gatekeep it (hehe) to the original developer. It’s just asking Apple to run some checks on the binary, that’s all.
There's no different; it doesn't matter.
That statement is not a complaint about notarization nor am I saying it is a reason to not notarize. It is simply an observation that I found surprising. If that were common knowledge, I would have assumed someone would have
...
💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515273927)
> Just clicking the cirrus links here it's hard to tell what the nature of these failures is, what may be causing them, and whether they happen reliably or randomly.
The nature of the failures is:
* They are caused by Wine.
* They happen randomly.
* They are known for years (A random mention of them from 2017 is https://github.com/bitcoin/bitcoin/blame/8e02b480591008c457cc841050f6f755ff54fcba/test/util/test_runner.py#L162). If you use a search engine you may find more or earlier mentions
...
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515273927)
> Just clicking the cirrus links here it's hard to tell what the nature of these failures is, what may be causing them, and whether they happen reliably or randomly.
The nature of the failures is:
* They are caused by Wine.
* They happen randomly.
* They are known for years (A random mention of them from 2017 is https://github.com/bitcoin/bitcoin/blame/8e02b480591008c457cc841050f6f755ff54fcba/test/util/test_runner.py#L162). If you use a search engine you may find more or earlier mentions
...
💬 theuni commented on pull request "cmake: Add `CheckLinkerSupportsPIE` module":
(https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092)
> This seems very fragile and unintuitive, and the fact that this could silently break at any point is not documented in any way. I don't think other bad design decisions should lead to us having to write even more boilerplate code to fix things that should "just work" (minus the upstream bugs).
Agreed. I forgot that we set `CMAKE_TRY_COMPILE_TARGET_TYPE` globally. And even worse, it's buried in a module. If that upsets CMake internal tests, I think we should undo that.
Could we invert thi
...
(https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092)
> This seems very fragile and unintuitive, and the fact that this could silently break at any point is not documented in any way. I don't think other bad design decisions should lead to us having to write even more boilerplate code to fix things that should "just work" (minus the upstream bugs).
Agreed. I forgot that we set `CMAKE_TRY_COMPILE_TARGET_TYPE` globally. And even worse, it's buried in a module. If that upsets CMake internal tests, I think we should undo that.
Could we invert thi
...