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

💬 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