💬 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
...
💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123471)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123471)
Fixed.
💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270125479)
@ajtowns The old code here first added a SEND, and then a RECEIVE, so if both were performed, only a SEND would be present in the map, meaning an unconditional send.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270125479)
@ajtowns The old code here first added a SEND, and then a RECEIVE, so if both were performed, only a SEND would be present in the map, meaning an unconditional send.
💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270127032)
I could reproduce the problem in a functional test today - for me, neither version (including the current one) fixes the problem completely. Will post detailed results soon.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270127032)
I could reproduce the problem in a functional test today - for me, neither version (including the current one) fixes the problem completely. Will post detailed results soon.
💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644864253)
I wrote a functional test, see https://github.com/mzumsande/bitcoin/tree/test_sipa_netstalling (because of the 1st commit obviously not intended for merge, but it makes it possible to reproduce the problem).
It works by mining a few large blocks, and having two nodes exchange these blocks in both direction by repeated `getblockfrompeer` messages, and then check whether the deadlock happened.
Unfortunately, the current branch doesn't appear to fix the problem completely, the test fails for
...
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644864253)
I wrote a functional test, see https://github.com/mzumsande/bitcoin/tree/test_sipa_netstalling (because of the 1st commit obviously not intended for merge, but it makes it possible to reproduce the problem).
It works by mining a few large blocks, and having two nodes exchange these blocks in both direction by repeated `getblockfrompeer` messages, and then check whether the deadlock happened.
Unfortunately, the current branch doesn't appear to fix the problem completely, the test fails for
...
💬 jonatack commented on pull request "test: update tool_wallet coverage for unexpected writes to wallet":
(https://github.com/bitcoin/bitcoin/pull/28116#issuecomment-1644870154)
Pulled in 2 commits from #27850 by @pinheadmz that this is now based on. Will rebase once that PR is merged.
(https://github.com/bitcoin/bitcoin/pull/28116#issuecomment-1644870154)
Pulled in 2 commits from #27850 by @pinheadmz that this is now based on. Will rebase once that PR is merged.
💬 ariard commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1270139854)
A comment can be added to precise this is the global not per-peer limit, at least on how it is used by `LimitOrphans()` (`DEFAULT_MAX_ORPHAN_TRANSACTIONS`).
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1270139854)
A comment can be added to precise this is the global not per-peer limit, at least on how it is used by `LimitOrphans()` (`DEFAULT_MAX_ORPHAN_TRANSACTIONS`).