💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212129551)
I mean that the computation won't be repeated if called a second time (due to self.den being set to 1, which triggers the fast path). Whether that's accomplished through actually remembering the int-converted form or some other mechanism is an implementation detail a user of the class shouldn't need to care about.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212129551)
I mean that the computation won't be repeated if called a second time (due to self.den being set to 1, which triggers the fast path). Whether that's accomplished through actually remembering the int-converted form or some other mechanism is an implementation detail a user of the class shouldn't need to care about.
💬 MarcoFalke commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#issuecomment-1570717706)
lgtm ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3
(https://github.com/bitcoin/bitcoin/pull/27783#issuecomment-1570717706)
lgtm ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3
🤔 Sjors reviewed a pull request: "Draft: rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1453921658)
If you can split 534b6f99a3779465678f39ed2704da6049c6469d in ~half that would make it a bit easier to review. Maybe start with anything that's trivial to move, and then do the more involved changes in the second commit.
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1453921658)
If you can split 534b6f99a3779465678f39ed2704da6049c6469d in ~half that would make it a bit easier to review. Maybe start with anything that's trivial to move, and then do the more involved changes in the second commit.
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212154654)
534b6f99a3779465678f39ed2704da6049c6469d: this function seems to come out of nowhere and is only used in a test.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212154654)
534b6f99a3779465678f39ed2704da6049c6469d: this function seems to come out of nowhere and is only used in a test.
💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570744518)
> @ryanofsky Are you planning on making any further changes here?
I'm not working on anything. The only suggestion I saw was to rename the `fr` variable, but that is a preexisting variable so would prefer not to change that here.
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570744518)
> @ryanofsky Are you planning on making any further changes here?
I'm not working on anything. The only suggestion I saw was to rename the `fr` variable, but that is a preexisting variable so would prefer not to change that here.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163089)
I guess we should; the parentheses are for listing parent classes, but I believe that an empty list or no parentheses at all don't make a difference.
Fixed.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163089)
I guess we should; the parentheses are for listing parent classes, but I believe that an empty list or no parentheses at all don't make a difference.
Fixed.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212164841)
I've added a `"no overflow allowed"` to the comment.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212164841)
I've added a `"no overflow allowed"` to the comment.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212166978)
I've kept the `FE.is_square()` function, but replaced it with just a `x.sqrt() is not None` plus a comment that a more efficient algorithm is possible.
This leaves the option of easily adding a more efficient implementation back later if really worth it.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212166978)
I've kept the `FE.is_square()` function, but replaced it with just a `x.sqrt() is not None` plus a comment that a more efficient algorithm is possible.
This leaves the option of easily adding a more efficient implementation back later if really worth it.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163658)
Ok, I added back.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163658)
Ok, I added back.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212172722)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212172722)
Fixed.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1570755789)
Addressed review comments. I've also renamed `FE.num` and `FE.den` to `FE._num` and `FE._den` to mark them as private (to the extent that Python allows that). All interactions with these objects should be done through their methods.
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1570755789)
Addressed review comments. I've also renamed `FE.num` and `FE.den` to `FE._num` and `FE._den` to mark them as private (to the extent that Python allows that). All interactions with these objects should be done through their methods.
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570774397)
Ah okay, I wasn't sure if you were going to add the named-only parameters to the conversion table, but based on the above discussion, it sounds like we're going to move towards eliminating the table in a followup.
ACK 2808c33bae95a0d2516a6e9a550032af142a04de
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570774397)
Ah okay, I wasn't sure if you were going to add the named-only parameters to the conversion table, but based on the above discussion, it sounds like we're going to move towards eliminating the table in a followup.
ACK 2808c33bae95a0d2516a6e9a550032af142a04de
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212164200)
534b6f99a3779465678f39ed2704da6049c6469d if it's broken in master, can we fix it _before_ refactoring? Alternatively there could be a `This will be fixed in the next commit` comment.
I'm also confused by the comment. `TryAddBlockIndexCandidate` is only called from `ReceivedBlockTransactions` after it sets `BLOCK_VALID_TRANSACTIONS`. So are you saying we're currently too strict? Was there a regression somewhere?
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212164200)
534b6f99a3779465678f39ed2704da6049c6469d if it's broken in master, can we fix it _before_ refactoring? Alternatively there could be a `This will be fixed in the next commit` comment.
I'm also confused by the comment. `TryAddBlockIndexCandidate` is only called from `ReceivedBlockTransactions` after it sets `BLOCK_VALID_TRANSACTIONS`. So are you saying we're currently too strict? Was there a regression somewhere?
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212161483)
534b6f99a3779465678f39ed2704da6049c6469d: maybe use this refactor as an opportunity to add a comment explaining the somewhat obtuse line above
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212161483)
534b6f99a3779465678f39ed2704da6049c6469d: maybe use this refactor as an opportunity to add a comment explaining the somewhat obtuse line above
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212173301)
534b6f99a3779465678f39ed2704da6049c6469d: `"active" :"background"`?
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212173301)
534b6f99a3779465678f39ed2704da6049c6469d: `"active" :"background"`?
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212186629)
534b6f99a3779465678f39ed2704da6049c6469d Previously `LoadExternalBlockFile` was only called on the `ActiveChainstate()`, now you're looping over both. This is ok? If possible, it would be good to move the change to `LoadExternalBlockFile` into a separate commit.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212186629)
534b6f99a3779465678f39ed2704da6049c6469d Previously `LoadExternalBlockFile` was only called on the `ActiveChainstate()`, now you're looping over both. This is ok? If possible, it would be good to move the change to `LoadExternalBlockFile` into a separate commit.
💬 pinheadmz commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1570783813)
> No not specifically. I was just wondering why there is an import_wallet rpc that accepts a filename where the file is on the system itself.
@brandonpille is this still an issue for you?
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1570783813)
> No not specifically. I was just wondering why there is an import_wallet rpc that accepts a filename where the file is on the system itself.
@brandonpille is this still an issue for you?
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1211727420)
In commit "walletdb: Consistently clear key and value streams before writing" (5d74d702c16f4e34c96e9573049a2fe930ac935c)
Right now it seems like there is not test coverage for calling GetNewPrefixCursor with an empty prefix. Also, the `\xff\xff` test case I previously suggested is not testing what I originally thought it would test. Because of the way strings are serialized, there's a compact int value before the prefix so the prefix has other characters beside `\xff`.
Seeing these things,
...
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1211727420)
In commit "walletdb: Consistently clear key and value streams before writing" (5d74d702c16f4e34c96e9573049a2fe930ac935c)
Right now it seems like there is not test coverage for calling GetNewPrefixCursor with an empty prefix. Also, the `\xff\xff` test case I previously suggested is not testing what I originally thought it would test. Because of the way strings are serialized, there's a compact int value before the prefix so the prefix has other characters beside `\xff`.
Seeing these things,
...
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1211714690)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (011087176e8aa3ec86f032c3e4c6bac432d0ac3e)
This change seems like it might belong in the previous commit "walletdb: Consistently clear key and value streams before writing" (5b5c131f9665e21f6dcf109e400926d054dd1fb5)
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1211714690)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (011087176e8aa3ec86f032c3e4c6bac432d0ac3e)
This change seems like it might belong in the previous commit "walletdb: Consistently clear key and value streams before writing" (5b5c131f9665e21f6dcf109e400926d054dd1fb5)
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1211828935)
In commit "walletdb: Consistently clear key and value streams before writing" (5d74d702c16f4e34c96e9573049a2fe930ac935c)
This test case seems to rely on getting values back in a particular order, but I don't think the sqlite implementation actually guarantees rows will be returned in any particular order, since no sorting is requested.
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1211828935)
In commit "walletdb: Consistently clear key and value streams before writing" (5d74d702c16f4e34c96e9573049a2fe930ac935c)
This test case seems to rely on getting values back in a particular order, but I don't think the sqlite implementation actually guarantees rows will be returned in any particular order, since no sorting is requested.