💬 john-moffett commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1429905802)
Thanks for the feedback!

I agree that it likely will only affect a very small proportion of users, but given that Bitcoiners can be somewhat peculiar, it's impossible to be certain. Since it's a non-destructive change, I think it's acceptable to just return a detailed error message for now. If it turns out that it affects more people than expected, we can add a more user-friendly process.

If nobody has any objections, I'll work on a commit that adds a detailed error message suggesting tha
...

💬 fanquake commented on pull request "build: prune Boost headers in depends":
(https://github.com/bitcoin/bitcoin/pull/24742#issuecomment-1429926358)
Dropped one additional unused header.

📝 fanquake opened a pull request: "refactor: don't avoid sys/types.h on when building for Windows"
(https://github.com/bitcoin/bitcoin/pull/27098)
We've already used it unguarded in `httpserver.cpp` for years, with no build issues.

Doesn't touch the usage in wallet/bdb.cpp. See #26832.

💬 fanquake commented on pull request "compat: move (win) S_* defines into bdb":
(https://github.com/bitcoin/bitcoin/pull/26832#issuecomment-1429935927)
Rebased on #27098.

💬 willcl-ark commented on issue "Expose PSBT AddInput/AddOutput via RPC":
(https://github.com/bitcoin/bitcoin/issues/19608#issuecomment-1429955448)
ISTM that although these methods could _perhaps_ be added in some way (so long as they do some verification on whether there were any signatures which had signed with `SIGHASH_ALL` already in the PSBT before proceeding?) there is not a lot of appetite to add them as they are very likely to cause issues.

@achow101 are we leaving this issue open as there's still a possibility of these RPCs being added?

💬 hebasto commented on pull request "refactor: don't avoid sys/types.h on when building for Windows":
(https://github.com/bitcoin/bitcoin/pull/27098#issuecomment-1429957486)
> don't avoid sys/types.h on when building for Windows

Does Windows builds require any symbols from `<sys/types.h>`? If not, why churn the code?

💬 fanquake commented on pull request "refactor: don't avoid sys/types.h on when building for Windows":
(https://github.com/bitcoin/bitcoin/pull/27098#issuecomment-1429961005)
> Does Windows builds require any symbols from <sys/types.h>?

Aside from ultimately removing pointless per-platform conditionals,

> See https://github.com/bitcoin/bitcoin/pull/26832.

💬 darosior commented on issue "Expose PSBT AddInput/AddOutput via RPC":
(https://github.com/bitcoin/bitcoin/issues/19608#issuecomment-1429962051)
For the record, i don't remember exactly why i advocated for this back when it was opened. I don't see why an application would need to use the `bitcoind` RPC to add inputs / outputs to a PSBT.

💬 ryanofsky commented on issue "odd behaviour of GetDataDir creating wallets/ subdirectory":
(https://github.com/bitcoin/bitcoin/issues/16220#issuecomment-1429962692)
I found this in my TODO notes from a previous discussion about this (https://github.com/bitcoin/bitcoin/pull/24266#discussion_r800865220), and think it could be a good solution

- `util/system` datadir code should not care about wallets or look for or create any wallet paths
- If `-walletdir` is specified, wallet code should use that directory to locate and list wallets.
- If `-walletdir` is specified and path is not a directory, wallet init should return a fatal error and refuse to start
-
...

💬 mzumsande commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#issuecomment-1429968540)
> which you've probably already caught

I hadn't, thanks for noticing! Fixed.

💬 hebasto commented on pull request "refactor: don't avoid sys/types.h when building for Windows":
(https://github.com/bitcoin/bitcoin/pull/27098#issuecomment-1429969983)
> Aside from ultimately removing pointless per-platform conditionals,

But this change does not remove per-platform conditionals.

> See #26832.

Yes, I agree with cc16ab13f42b83672030fdd03c65fe27699854ee from #26832.

💬 jonatack commented on pull request "Modernize rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/27081#issuecomment-1429971743)
> tACK Tested as follows: generated new userpw using an explicit password. Used it on signet through an RPC request (checked I got 'incorrect password attempt' if I changed the userpw a bit).

@codo1 If this comment can be helpful, your review is listed as ignored in https://github.com/bitcoin/bitcoin/pull/27081#issuecomment-1426430503 and is not part of the merge commit because your ACK isn't followed by the commit hash. See these links for more information:

- https://jonatack.github.io/a
...

💬 fanquake commented on pull request "refactor: don't avoid sys/types.h when building for Windows":
(https://github.com/bitcoin/bitcoin/pull/27098#issuecomment-1429971818)
> But this change does not remove per-platform conditionals.

Yea, that's because this is a step towards doing so.

💬 fanquake commented on pull request "contrib: Check symbols in the `bitcoinconsensus` shared library":
(https://github.com/bitcoin/bitcoin/pull/25020#discussion_r1106015544)
NACK. This is accomodating a bug (in the DLL), and doing so in such a way, that it could leak into the actual binaries.

💬 hebasto commented on pull request "contrib: Check symbols in the `bitcoinconsensus` shared library":
(https://github.com/bitcoin/bitcoin/pull/25020#discussion_r1106019516)
> This is accomodating a bug...

Which one?

💬 fanquake commented on pull request "contrib: Check symbols in the `bitcoinconsensus` shared library":
(https://github.com/bitcoin/bitcoin/pull/25020#discussion_r1106020657)
Non-static libssp.

💬 ryanofsky commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1429983108)
I could be wrong, but the current version of this 41e9a810837b1c1eb497c2c22a6fa9873f379836 appears to change behavior by only creating the `<datadir>/<netdir>/wallets` directory, and no longer creating the `<datadir>/wallets` one level up if `<datadir>` did not exist.

This seems like a potentially bad thing, because if testnet or signet or regtest bitcoin is started mainnet bitcoin, mainnet wallets will go be created in `<datadir>` not `<datadir>/wallets` as intended. I think more ideally thi
...

📝 fanquake opened a pull request: "build: produce a .zip for macOS distribution"
(https://github.com/bitcoin/bitcoin/pull/27099)
Reviving the discussion around using a `.zip` for the distributed macOS binaries, as opposed to a `.dmg`.

The commits can be improved, but currently good enough for (non-guix) testing / discussion.

Given we only had a single report of the "no finder window" issue (#26176), I wonder if that means macOS users were able to figure it out, they gave up/didn't report, or, we just have very few macOS users.

Related to #18128.

💬 fanquake commented on issue "build: Use zip instead of dmg for macOS":
(https://github.com/bitcoin/bitcoin/issues/18128#issuecomment-1429990669)
Opened a new PR to discuss this change in #27099.

💬 achow101 commented on issue "Expose PSBT AddInput/AddOutput via RPC":
(https://github.com/bitcoin/bitcoin/issues/19608#issuecomment-1430001293)
They could be added with after PSBTv2 (once #21283 is merged) since PSBTv2 actually supports this operation.