💬 jonatack commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269874331)
It looks like the PR description section about `skip_if_root` is now out of date.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269874331)
It looks like the PR description section about `skip_if_root` is now out of date.
💬 jonatack commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269876108)
If you retouch, I think the logging for the assertions inside this `is_unix_root` conditional should be logged as `info` (maybe all the debug logging in this file currently could be info).
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269876108)
If you retouch, I think the logging for the assertions inside this `is_unix_root` conditional should be logged as `info` (maybe all the debug logging in this file currently could be info).
💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644484394)
FWIW, I assume this issue has existed since this code was introduced, but isn't harmful apart from preventing compilation, as far as I can tell.
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644484394)
FWIW, I assume this issue has existed since this code was introduced, but isn't harmful apart from preventing compilation, as far as I can tell.
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269884246)
thank you., fixed
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269884246)
thank you., fixed
💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644486605)
I don't actually think it even prevents compilation, and only warns? Unclear from the issue.
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644486605)
I don't actually think it even prevents compilation, and only warns? Unclear from the issue.
💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644487671)
> I don't actually think it even prevents compilation, and only warns? Unclear from the issue.
Only warnings. Compilation succeeds.
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644487671)
> I don't actually think it even prevents compilation, and only warns? Unclear from the issue.
Only warnings. Compilation succeeds.
💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644488214)
Errr, if it doesn't prevent compilation it's possible that the result is wrong. Do tests pass?
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644488214)
Errr, if it doesn't prevent compilation it's possible that the result is wrong. Do tests pass?
💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644488972)
> Errr, if it doesn't prevent compilation it's possible that the result is wrong (when the warning is emitted). Do tests pass?
They do.
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644488972)
> Errr, if it doesn't prevent compilation it's possible that the result is wrong (when the warning is emitted). Do tests pass?
They do.
💬 MarcoFalke commented on pull request "ci: Set DEBUG=1 for valgrind fuzz task":
(https://github.com/bitcoin/bitcoin/pull/28106#issuecomment-1644495948)
Looks like this will be too slow for the functional tests, and there are other tasks setting it, so I've only added it to the fuzz valgrind task, which had it not previously set.
An alternative might be to add a new fuzz task with just DEBUG=1 set?
(https://github.com/bitcoin/bitcoin/pull/28106#issuecomment-1644495948)
Looks like this will be too slow for the functional tests, and there are other tasks setting it, so I've only added it to the fuzz valgrind task, which had it not previously set.
An alternative might be to add a new fuzz task with just DEBUG=1 set?
💬 MarcoFalke commented on pull request "ci: Set DEBUG=1 for valgrind fuzz task":
(https://github.com/bitcoin/bitcoin/pull/28106#issuecomment-1644514902)
Maybe I'll just do it in another repo, instead of here. Closing for now.
(https://github.com/bitcoin/bitcoin/pull/28106#issuecomment-1644514902)
Maybe I'll just do it in another repo, instead of here. Closing for now.
✅ MarcoFalke closed a pull request: "ci: Set DEBUG=1 for valgrind fuzz task"
(https://github.com/bitcoin/bitcoin/pull/28106)
(https://github.com/bitcoin/bitcoin/pull/28106)
💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1644517170)
crACK fa633aa
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1644517170)
crACK fa633aa
💬 brunoerg commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269917734)
After running it for some time, the maximum exec/s I got was 21 exec/s which is extremely low. It's not all the targets that reach more than 1000 exec/s here but 21 is too slow.
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269917734)
After running it for some time, the maximum exec/s I got was 21 exec/s which is extremely low. It's not all the targets that reach more than 1000 exec/s here but 21 is too slow.
💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644559652)
In that case it seems to be doing the right thing despite the warning.
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644559652)
In that case it seems to be doing the right thing despite the warning.
💬 jonatack commented on pull request "test: update tool_wallet coverage for unexpected writes to wallet":
(https://github.com/bitcoin/bitcoin/pull/28116#discussion_r1269940082)
Thanks for reviewing! Done.
(https://github.com/bitcoin/bitcoin/pull/28116#discussion_r1269940082)
Thanks for reviewing! Done.
💬 brunoerg commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269953546)
In 55c84c2d3bff09784ad127aba68a166f3f36f215: Is there any specific reason for this approach - Instead of using a fixed value like `process_message` does?
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269953546)
In 55c84c2d3bff09784ad127aba68a166f3f36f215: Is there any specific reason for this approach - Instead of using a fixed value like `process_message` does?
🤔 theStack reviewed a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1539997888)
Reviewed the first four commits so far (up to c61fa6ab5917f3745a917f4fc2f3a9e96629d5d9), verified that the implementations of `AEADChaCha20Poly1305`, `FSChaCha20` and `FSChaCha20Poly1305` match the corresponding RFC8439 / BIP324 specifications. As with previous PRs, verified the test vectors (this time using PyCryptodome again) which was a bit more effort this time, as the rekeying wrappers FSChaCha20{Poly1305} also had to be implemented first: https://gist.github.com/theStack/e910505e39204c695a
...
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1539997888)
Reviewed the first four commits so far (up to c61fa6ab5917f3745a917f4fc2f3a9e96629d5d9), verified that the implementations of `AEADChaCha20Poly1305`, `FSChaCha20` and `FSChaCha20Poly1305` match the corresponding RFC8439 / BIP324 specifications. As with previous PRs, verified the test vectors (this time using PyCryptodome again) which was a bit more effort this time, as the rekeying wrappers FSChaCha20{Poly1305} also had to be implemented first: https://gist.github.com/theStack/e910505e39204c695a
...
💬 theStack commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1269911157)
(in commit c61fa6ab5917f3745a917f4fc2f3a9e96629d5d9)
nit: these refactoring changes in `UpdateTag` seem to be unrelated to FSChaCha20Poly1305, I think they can be already included the commit that introduces the ChaCha20Poly1305 AEAD (to avoid touching it again later and keep the diff small)? Also, `aad_padding` and `cipher_padding` could be const.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1269911157)
(in commit c61fa6ab5917f3745a917f4fc2f3a9e96629d5d9)
nit: these refactoring changes in `UpdateTag` seem to be unrelated to FSChaCha20Poly1305, I think they can be already included the commit that introduces the ChaCha20Poly1305 AEAD (to avoid touching it again later and keep the diff small)? Also, `aad_padding` and `cipher_padding` could be const.
💬 theStack commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1269960092)
nit: could encrypt _after_ generating the poly1305 key to avoid having to `Seek64` twice, as the chacha20 object is already at the desired block count 1 after the `Keystream` call below (I think generate-poly1305-key -> encrypt -> compute tag is also the order as described in RFC8439). On the other hand, performance-wise it shouldn't make a difference as `Seek64` is quite cheap, and maybe it's even preferred to be explicit about the block counter instead. I haven't looked at any other ChaCha20Po
...
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1269960092)
nit: could encrypt _after_ generating the poly1305 key to avoid having to `Seek64` twice, as the chacha20 object is already at the desired block count 1 after the `Keystream` call below (I think generate-poly1305-key -> encrypt -> compute tag is also the order as described in RFC8439). On the other hand, performance-wise it shouldn't make a difference as `Seek64` is quite cheap, and maybe it's even preferred to be explicit about the block counter instead. I haven't looked at any other ChaCha20Po
...
💬 theStack commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1269973943)
nit: it might be worth introducing a generate-poly1305-key helper (or even one that also does the tag computation already, given also aad and ciphertext) that can be called in Encrypt and Decrypt, to deduplicate code?
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1269973943)
nit: it might be worth introducing a generate-poly1305-key helper (or even one that also does the tag computation already, given also aad and ciphertext) that can be called in Encrypt and Decrypt, to deduplicate code?