📝 theStack opened a pull request: "test: check that sigop limit also affects ancestor/descendant size (27171 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/27265)
This is a follow-up to #27171, adding a check that the sigop-limit vsize logic is also respected for {ancestor,descendant}size calculation (as suggested in https://github.com/bitcoin/bitcoin/pull/27171#pullrequestreview-1331143909). For simplicity, we use a one-parent-one-child cluster here and only check for the case that the sigop-limit equivalent size is larger than the serialized vsize.

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137601762)
> hm, seems a little low-level for release notes. maybe something like

sounds much better! thank you!

> the target group for release notes are regular users, while test-only options are meant for devs and may not be stable.

interesting, thanks! i've updated it.

💬 TheCharlatan commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1137603241)
ACK, this is resolved.

💬 TheCharlatan commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1137603410)
ACK, this is resolved.

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137605901)
makes sense, sporadic failures do happen in that commit (removed it now).
continued this discussion here - https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137596851

💬 brunoerg commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137608018)
What's the reason for `-cjdnsreachable` here?

💬 stratospher commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1470614184)
I wasn't able to answer this https://github.com/bitcoin/bitcoin/pull/19860#discussion_r829878150. (so didn't include it)
Do you(/anyone) know why that commit is needed?

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137617169)
oops, forgot to revert that. thanks!

💬 brunoerg commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137621405)
ohh, got it! thank you!

💬 ryanofsky commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470663620)
> +1 PR in progress on this branch: https://github.com/pinheadmz/bitcoin/commits/conf-file

Thanks, just some quick feedback on 2a2488fab691a69b66395929a2c645b4a2649ace: I don't think it would be good to add a new level of settings precedence just to deal with this one GUI setting. We migrated all settings out of QSettings which are not GUI specific in https://github.com/bitcoin-core/gui/pull/602 and we can migrate `strDataDir` as well to avoid unnecessary differences between CLI and GUI behav
...

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1137647274)
> I strongly dislike using the empty string to indicate deletion. I think it would be better to have a separate `RemoveReceiveRequest`.

You are right. This is shitty and I will add a separate method to avoid continuing this pattern.

💬 hebasto commented on pull request "Correct poor grammar in wallet synchronization warning.":
(https://github.com/bitcoin-core/gui/pull/720#issuecomment-1470692898)
Translation files should be modified during the translation process only. Please drop those changes from this PR.

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1137679417)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1137458690

> I'm not convinced that this needs to be a map. Receive requests are only created by the GUI, and the GUI only creates them for a newly retrieved address, so it shouldn't be possible to have multiple receive requests.
>
> Additionally, receive requests are identified by an int which is being (un)stringified. This could just be an int to avoid that constant conversion in the GUI.

You could definitely be right about
...

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1137722557)
Moved it to a separate function, although I did not split it up into multiple as that does not seem to be actually useful. Also added some additional comments.

👍 ryanofsky approved a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)
Code review ACK db521d3e89b23046f73410a875bb9660178e3842. Thanks for adopting suggested changes. I left some new suggestions, but they are not important and can be ignored.

I think this PR should be straightforward for others to review now, and nice because it should demystify the purpose field and wallet address book as a whole.

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137726714)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

Would maybe add a comment like "// In very old wallets, address purpose may not recorded, so derive it from IsMine value"

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137712523)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

Note for reviewers: in a few places in this PR, struct order or argument order is changed so the `AddressPurpose` field follows IsMine or IsChange fields. This is done because the fields hold overlapping information, and in most cases new code should rely on the other fields instead of `AddressPurpose`.

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137728596)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

Could add the same comment here ("In very old wallets, address purpose may not recorded, so derive it from IsMine value")

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137732694)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

Note for reviewers: this is preserving previous behavior. Default value for `CAddressBook::purpose` field previously was `"unknown"` when a purpose was not recorded, and now it is null which is translated to `"unknown"`

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137736576)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

If not updating the code above, would be good to add `assert(purpose)` or a similar check here, because that condition would indicate a bug.