💬 ryanofsky commented on pull request "Add fee_est tool for debugging fee estimation code":
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1529830672)
@josie thanks for looking at this. I don't think I understand what the idea behind an approach NACK would be based on what you are saying, though.

It sounds more like you're asking for an additional feature which would be easy to implement on top of the first commit, not pointing to a drawback of the approach. If you want to gather live fee estimates, the first commit outputs a structured [jsonl](https://jsonlines.org/) log of fee estimation events, so it would be easy to add the actual fee e
...

💬 pinheadmz commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1181656908)
I was momentarily confused by this because (IIUC) map keys are the `int64_t id` field of `RecentRequestEntry` but actually encoded into `std::string` by a few methods in `RecentRequestsTableModel`. So I'm wondering: Is there a reason we need to keep those IDs as strings? Is it like, at this point, going back to int64 would be a needless refactor?

💬 jarolrod commented on pull request "qt: 25.0rc2 translations update":
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1529845383)
Concept ACK, could use an update?

💬 RandyMcMillan commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1529846864)
Concept ACK

💬 RandyMcMillan commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#issuecomment-1529850188)
Concept ACK

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

> So I'm wondering: Is there a reason we need to keep those IDs as strings? Is it like, at this point, going back to int64 would be a needless refactor?

Only reason that the [`CAddressBookData::receive_requests`](https://github.com/bitcoin/bitcoin/blob/be0325c6a62505d63bc07320b05e31618ef9bbb1/src/wallet/wallet.h#L232-L239) map containing the requests has string keys and string values is that the [`setAddressReceiveReq
...

💬 pinheadmz commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#issuecomment-1529982632)
> Concept ACK

Thanks Randy! I just rebased on master to fix merge conflict. If you'd like to see this move forward, you can help my reviewing the parent Bitcoin Core PR: https://github.com/bitcoin/bitcoin/pull/27216 🙏 ❤️

💬 pinheadmz commented on issue "v 0.17.0 win 7 64 bit missing recent transactions. ":
(https://github.com/bitcoin/bitcoin/issues/14541#issuecomment-1529990436)
@MarcoFalke I think [this comment](https://github.com/bitcoin/bitcoin/issues/14541#issuecomment-621434177) made us think it was not the wallet's fault but the p2p stalling issue. @VadimEv are you able to test again with the latest release of Bitcoin Core?

👍 ryanofsky approved a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1407882016)
Code review ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9. Added new assert, test, and comment since last review

💬 ryanofsky commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1181752551)
Makes sense the `+1` is needed to be consistent with `>=` below. It does look like there is a minor bug here that will cause block files to be at most `MAX_BLOCKFILE_SIZE-1` bytes and never reach the maximum size. But if this is a bug, it goes back to 5382bcf8cd23c36a435c29080770a79b5e28af42 when this code was introduced. Agree it would not be good to change that behavior here.

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1181755516)
Agree with the idea of programmatic calculation. I noticed [NET_MAX](https://github.com/bitcoin/bitcoin/blob/v25.0rc1/src/netaddress.h#L68) in `netaddress.h` accomplishes this by adding an enum value to the end of the list. That new value serves as a counter for all the enums. I initially implemented these (number of connection and message types) in that same manner because it seemed more programmatic. As long as new enums were inserted before, then I figured they would be accounted for in the t
...

💬 pinheadmz commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1530010980)
@Tracachang We're going to close this issue as "won't fix" for now. I'm going to follow up in https://github.com/bitcoin/bitcoin/issues/9492 where I see you've already chimed in. I think a simple doc outlining the list/import descriptor commands I commented above and using PSBT for signing would be the most valuable.

As far as the missing taproot addresses in the GUI you've reported - that could be opened as a new issue if you can provide steps to reproduce it with the latest release of Bitco
...

✅ pinheadmz closed an issue: "Export a watch wallet only (with descriptors and without private keys) for an air gap setup"
(https://github.com/bitcoin/bitcoin/issues/24829)

💬 pinheadmz commented on issue "Write instructions on offline signing.":
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1530018625)
> Hello, I wrote a step by step [tutorial on bitcointalk forum](https://bitcointalk.org/index.php?topic=5392824.msg59740404#msg59740404) about offline signing one year ago if it can help.
>
> Screenshots are not online any more but I do still have them on another website if needed.

Hey I just took a quick look at this guide and it looks nice and simple. I think it would make a nice addition to the core docs, with links to & from some of current docs like `psbt.md` `external-signer.md` and
...

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1181764326)
Ooooo thank you! I never would have caught this. Fixed and rebased: https://github.com/bitcoin/bitcoin/pull/27534/commits/5ed0c78efd9e43bcb84d85c257081e224cf5e69f

💬 hebasto commented on pull request "qt: 25.0rc2 translations update":
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1530026783)
> could use an update?

Updated.

👍 TheCharlatan approved a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1407905862)
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9

💬 pinheadmz commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1181780876)
The functional test covers the `+1` nicely as well. Without the `+1` (and removing the assertion on L632) the test will hang until timed out at 30s

👍 pinheadmz approved a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1407925093)
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRQBA4ACgkQ5+KYS2KJ
yTr14BAAw1BUdgVGe3UKG5epiL1XsijS0stgQRrXVwPJKM/7O4nAy5LRQBx/ASB+
+xH0CGkd8dULO9yPrXZHXn2tjlv36Wde+ECHy76BRy6qPYgQYf/KfX0RKl7IUw3D
RNTXxYPOopXDO9h9U2jmQpwZDU7Ri3DOD5BujVq+oAu0a+782QvoTMo/Yv9mEiKr
JOyNhSzC
...

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1181791254)
I agree with the questioning of where these util methods belong.

I didn't know about `[[nodiscard]]`. Cool feature! Added it to the public `*(to/from)Index` and private `*ToIndex` methods by this comment. New commit after update + rebase: https://github.com/bitcoin/bitcoin/pull/27534/commits/fc86267b0ece8d31915a4869741f4602622f2bcb