💬 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.

💬 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.

💬 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

💬 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?

💬 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

💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(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.

💬 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?

💬 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,
...

💬 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)

💬 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.

💬 pinheadmz commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1570789627)
I think this can be closed as solved by https://github.com/bitcoin/bitcoin/pull/27302 ?

💬 pinheadmz commented on issue "Relative paths named in the -conf parameter reset when parsing datadir in named config":
(https://github.com/bitcoin/bitcoin/issues/19990#issuecomment-1570790910)
Closed by https://github.com/bitcoin/bitcoin/pull/27302

✅ pinheadmz closed an issue: "Relative paths named in the -conf parameter reset when parsing datadir in named config"
(https://github.com/bitcoin/bitcoin/issues/19990)

💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1570801767)
Rebased

💬 achow101 commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212219391)
Done

💬 achow101 commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212219741)
Adopted these suggestions. I've put the bug fixes in an separate commit.

💬 achow101 commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212220552)
Hmm, indeed. I've changed the test to check against the `i` stored in the key.

🤔 mzumsande reviewed a pull request: "addrman: select addresses by network follow-up"
(https://github.com/bitcoin/bitcoin/pull/27745#pullrequestreview-1454032733)
Code Review ACK cd8ef5b3e66b3f766c9c883259b5feb44540d7df

💬 mzumsande commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#discussion_r1212228531)
I guess if the `Assume` assumption breaks the worst thing that could happen is that the while loop may run indefinitely - but it might also pick another address if there is one. So this seems better than crash with an assert.