💬 brunoerg commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#discussion_r1113366560)
doubt:
```
To create a 4 GiB RAM disk named "ramdisk" at `/Volumes/ramdisk/`:
```

Does it create a RAM disk named "ramdisk" at `/Volumes/`, resulting in `/Volumes/ramdisk/` or it creates a RAM disk named "ramdisk" into `/Volumes/ramdisk/`?

💬 kristapsk commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#issuecomment-1438851708)
Concept ACK

💬 achow101 commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#discussion_r1113391015)
Looking at the [source](https://github.com/git/git/blob/master/builtin/merge-tree.c), I believe it uses `ort`.

💬 Sjors commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1438875758)
I don't see any harm in adding the word "test". A user who searches "bitcoin" will see the "signet" entry in their search results and wonder what it is, perhaps even worry about it. Without the word "test" the only two ways to figure out are to google "what is signet" or to launch the application.

I don't expect most users to know what "mainnet" is. They obviously know what Bitcoin is, but they may not know that one or more test networks exist.

💬 MarcoFalke commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#discussion_r1113395319)
So the minimum git version will be 2.38? https://github.com/git/git/commit/1f0c3a29da3515d88537902cd267cc726020eea5

Might be good to test on commit 0cfbb171bd6f61a4edba913e281553b9bdf08d4a

💬 achow101 commented on issue "signrawtransactionwithkey command shouldn't output the "Witness program was passed an empty witness" error for a TapRoot transaction":
(https://github.com/bitcoin/bitcoin/issues/27017#issuecomment-1438882233)
`signrawtransactionwithkey` does not currently support taproot (and it is not clear whether it ever will) as there is not enough information provided with just a private key in order to be able to sign such transactions.

💬 1440000bytes commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1438890512)
> I don't expect most users to know what "mainnet" is. They obviously know what Bitcoin is, but they may not know that one or more test networks exist.

If someone doesnt know what is mainnet they wont use bitcoin core.

📝 jonatack opened a pull request: "rpc, test: remove newline escape sequence from wallet warning fields"
(https://github.com/bitcoin/bitcoin/pull/27138)
This PR extends our test coverage to demonstrate the issue, then removes newline escape sequences in the wallet `warning` field in RPCs `createwallet`, `loadwallet`, and `unloadwallet`.

I believe only `createwallet` appends additional warning sentences and is affected by the fix.

before

```
$ ./src/bitcoin-cli -signet createwallet w1 false false "" false false
{
"name": "w1",
"warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet created successfully
...

💬 1440000bytes commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1438895432)
Most of the contributors in this repo have no experience about windows including windows servers. Sorry for being rude or straight but its a fact. I love linux but if you support multiple OS which is expected then please understand its different from a user's perspective.

Luke Dashjr will be the last person to ask if I need to add something in core for windows.

I have worked on windows for a long time and love linux. You can ignore my suggestion and NACK. Merge it as other PRs however I kn
...

💬 pinheadmz commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#discussion_r1113408160)
@brunoerg

Not sure I understand. The volume is named "ramdisk" and is mounted at `/Volumes/ramdisk`. The path `/Volumes/ramdisk` does not exist before the command is executed.

💬 roconnor-blockstream commented on pull request "script: BIP341 txdata cannot be precomputed without spent outputs":
(https://github.com/bitcoin/bitcoin/pull/27122#issuecomment-1438911071)
> The immediately following comment seems wrong too, as the branch is gated by a !scriptWitness.IsNull() test?

> > Note that this branch may trigger for scriptPubKeys that aren't actually segwit but in that case validation will fail as SCRIPT_ERR_WITNESS_UNEXPECTED anyway.

I believe this comment is saying it will trigger in the case the the scriptPubKey is OP_1 followed by a series of opcodes of an appropriate length, but is not a single push opcode, and a witness is (illegally) provide
...

💬 roconnor-blockstream commented on pull request "script: BIP341 txdata cannot be precomputed without spent outputs":
(https://github.com/bitcoin/bitcoin/pull/27122#issuecomment-1438917640)
> This was directly addressed in the comments earlier; should that be updated too?

> > This only works if spent_outputs was provided as well, but if it wasn't, actual validation will fail anyway.

I believe this comment is trying to say that (under the implicit circumstances that `force` is false) if the spend outputs are not provided, it will fail to detect if an input is using bip341. But if spent outputs are not provided, then bip341 cannot be validated anyways.

💬 Sjors commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#discussion_r1113443957)
It seems so. If I remove the Homebrew version of Git (2.39.2) and fall back to Apple's default 2.37.1 the verification script fails on that commit:

```
% contrib/verify-commits/verify-commits.py 0cfbb17
Using verify-commits data from /Users/sjors/dev/bitcoin/contrib/verify-commits
usage: git merge-tree <base-tree> <branch1> <branch2>
Traceback (most recent call last):
File "contrib/verify-commits/verify-commits.py", line 191, in <module>
main()
File "contrib/verify-commits/ver
...

💬 achow101 commented on pull request "script: BIP341 txdata cannot be precomputed without spent outputs":
(https://github.com/bitcoin/bitcoin/pull/27122#issuecomment-1438942314)
ACK 95f12de92505522a32ba58acd5251c69e602d160

🚀 achow101 merged a pull request: "script: BIP341 txdata cannot be precomputed without spent outputs"
(https://github.com/bitcoin/bitcoin/pull/27122)

✅ achow101 closed an issue: "Add EnsureWalletIsUnlocked to rescanblockchain RPC?"
(https://github.com/bitcoin/bitcoin/issues/25702)

🚀 achow101 merged a pull request: "wallet: ensure the wallet is unlocked when needed for rescanning"
(https://github.com/bitcoin/bitcoin/pull/26347)

💬 sipa commented on pull request "fuzz: avoid redundant dup key checks when creating Miniscript nodes":
(https://github.com/bitcoin/bitcoin/pull/27117#issuecomment-1438974152)
ACK c1b7bd047f47dcd3eb6897adfaf9a55594deff5d

Ran fuzzing with miniscript_smart target, see https://github.com/bitcoin-core/qa-assets/pull/105.

👍 stickies-v approved a pull request: "wallet: SecureString to allow null characters"
(https://github.com/bitcoin/bitcoin/pull/27068)
ACK d73721b2be27afd9906ed362e286dd3d0a414413

2 non-blocking nits best to be ignored if no further force pushes are necessary.

---

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

Thanks, I didn't realize this. I verified it both on cli and Qt and you're right. I agree that the chances of this actually being a problem a
...

💬 stickies-v commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#discussion_r1113431868)
nit: slight rephrasing for tense consistency and highlighting that it's the old password that has the null character (also (partially) affects the other if-branch as well as the other sites where this message is raised).
```suggestion
tr("The old passphrase entered for the wallet decryption is incorrect. "
"It contains a null character (ie - a zero byte). "
```

💬 pinheadmz commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1113492544)
I think you are creating a wallet with the letter "t" in the name, but the path you pass to migratewallet has no "t":

`notloaded2` vs `noloaded2`

I think this isn't causing test failure because `test_unloaded_by_path()` is not added to the `run_test` list ...?