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

💬 brunoerg commented on pull request "wallet: clarify replace fields in gettransaction":
(https://github.com/bitcoin/bitcoin/pull/27782#issuecomment-1570940892)
I suppose that changing them in `TransactionDescriptionString` will also affect `listsinceblock` and `listtransactions`. So, I think would be appropriate to update the PR title/commit message?

🤔 brunoerg reviewed a pull request: "wallet: clarify replace fields in gettransaction"
(https://github.com/bitcoin/bitcoin/pull/27782#pullrequestreview-1454150449)
Concept ACK

💬 Macoophonic commented on pull request "wallet: clarify replace fields in gettransaction":
(https://github.com/bitcoin/bitcoin/pull/27782#issuecomment-1570963364)
Wallet code It's so dark, thank you
Được gửi từ Yahoo Mail trên Android

Vào Th 5, 1 thg 6, 2023 lúc 4:00, Bruno ***@***.***> đã viết:


@brunoerg commented on this pull request.

Concept ACK

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

👍 ryanofsky approved a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790#pullrequestreview-1454166240)
Code review ACK 13476fe7bebdbf51e09821850b2c808c8ecf116a

💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (13476fe7bebdbf51e09821850b2c808c8ecf116a)

Maybe add a comment above the batch.Write() call like "Convert the key and value to DataStream objects in order to bypass serialization. We want raw bytes to be written to the database, not serialized byte strings. The DatabaseBatch::Write template method normally serializes its arguments, but because DataStream has a Serialize method that does concatenation instead of serialization, it can be
...

💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570974530)
> I wasn't sure if you were going to add the named-only parameters to the conversion table

Sorry, I just missed the part of your earlier comment where you suggested to this. I didn't realize the conversion table could handle parameters not passed by position. Should be fixed now though.

Updated 2808c33bae95a0d2516a6e9a550032af142a04de -> 5559ad2c69ef82c18843b9214e5ba3974834a1ae ([`pr/nonly.15`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.15) -> [`pr/nonly.16`](https://github.com/
...