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

💬 achow101 commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1430018623)
> I googled "random password generator" and with the [second result](https://passwordsgenerator.net/) I was able to generate a null-character containing password (\039"`\9%|+']1+5)

But that doesn't actually contain a null character. That contains a backslash followed by a 0, which some things may interpret as a null character, but not always, and not necessarily in a way that will affect us. For example. my terminal will interpret it as a null character if I don't put quotes around it. It wil
...

👍 jonatack approved a pull request: "net: avoid overriding non-virtual ToString() in CService and use better naming"
(https://github.com/bitcoin/bitcoin/pull/25619)

📝 MarcoFalke opened a pull request: "ci: Add CLA bot"
(https://github.com/bitcoin/bitcoin/pull/27100)
Not sure for which files we want to enforce copyright headers, if any at all. However if we do it for any files, it should be enforced by CI. See also the section "Copyright" in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#copyright