💬 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?
💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-1644753444)
The commit message for 68bb1463dc1f995fbfdb75d3acef625bde104275 should be updated because now that #27145 has gotten merged, the described issue has already been fixed.
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-1644753444)
The commit message for 68bb1463dc1f995fbfdb75d3acef625bde104275 should be updated because now that #27145 has gotten merged, the described issue has already been fixed.
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270121452)
Not if you make it `WantsToSend() EXCLUSIVE_LOCK_REQUIRED(cs_vSend)` and require the caller to have the lock?
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270121452)
Not if you make it `WantsToSend() EXCLUSIVE_LOCK_REQUIRED(cs_vSend)` and require the caller to have the lock?
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270122517)
Err, doesn't that mean this behaved the same as the previous code? How did it [fix anything](https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1608257369)?
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270122517)
Err, doesn't that mean this behaved the same as the previous code? How did it [fix anything](https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1608257369)?
💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270122616)
Done, I've replaced `UpdateTag` with `ComputeTag`, which does the whole tag calculation, including poly1305 key generation.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270122616)
Done, I've replaced `UpdateTag` with `ComputeTag`, which does the whole tag calculation, including poly1305 key generation.
💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123417)
I believe it's better not to do that, though it really doesn't matter much.
The reason is that if we'd start by generating the key, we'd have to store that key in memory somewhere, leave it there for the whole encryption, then fetch it again (at which point it's quite possibly gone from CPU caches) to compute the tag. By seeking and deriving at the end, we only need the chacha20 key/state, which is likely still hot at that point (as it's needed every 64 bytes of encryption). And the seeking i
...
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123417)
I believe it's better not to do that, though it really doesn't matter much.
The reason is that if we'd start by generating the key, we'd have to store that key in memory somewhere, leave it there for the whole encryption, then fetch it again (at which point it's quite possibly gone from CPU caches) to compute the tag. By seeking and deriving at the end, we only need the chacha20 key/state, which is likely still hot at that point (as it's needed every 64 bytes of encryption). And the seeking i
...