💬 Empact commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#discussion_r1246797170)
Done

💬 Empact commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#issuecomment-1613406421)
Rebased. 👍

💬 fanquake commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1613444184)
Rebased on master, and put on top of #27999. Also swapped out the time-machine bumping commit to be the same one as from #27897.

👍 theStack approved a pull request: "test: add python implementation of Elligator swift"
(https://github.com/bitcoin/bitcoin/pull/24005#pullrequestreview-1505614994)
Code-review ACK afd762afa39569d8932dd0cf62f8d134e25fc651

Regarding https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1246752385, I've prepared at least adding the BIP324 decoding test vectors today (https://github.com/theStack/bitcoin/commit/06c942493af9df766f85dc1387daaa0441e7d10e). Feel free to pick it up either in this PR or a follow-up.

💬 theStack commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1246822528)
non-blocking nit:
```suggestion
self.assertEqual(shared_secret1, shared_secret2)
```
(slightly preferred, as in the case of a mismatch the values would be shown)

💬 sipa commented on pull request "refactor: Drop unsafe AsBytePtr function":
(https://github.com/bitcoin/bitcoin/pull/27978#issuecomment-1613459921)
utACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9

🤔 vasild reviewed a pull request: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1505515197)
64d0f234e9 looks good, except there is some messup in the contents of these two commits:

87064712899dacfac7f66f60c8ed1c0f4e65c24f `netbase: refactor CreateSock() to accept sa_family_t`
bbff39d3619428df3745d884ea36d7febff337b1 `netbase: extend Proxy class to wrap UNIX socket as well as TCP`

The first one introduces one more `IsValid()` method, in addition to the existent one. The new method uses a variable that does not exist: `is_unix_socket`, thus it will not compile.
Then the other com
...

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1246758964)
nit: it does not have to be `const` when passing by value - there is no way to for the function to modify the variable that is being passed (and have effects visible outside of the function).

```suggestion
std::unique_ptr<Sock> CreateSockOS(sa_family_t address_family);
```

See f3() in http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1246856004)
The new member `unix_socket_path` is still without `m_` prefix. Only `is_unix_socket` was renamed to `m_is_unix_socket`.

📝 fanquake opened a pull request: "doc: cleanup release process doc"
(https://github.com/bitcoin/bitcoin/pull/28003)
To-be-updated collection of changes to to simplify / correct the release-process documentation.
I think we could still simplify this further. For example, we could remove the guix building docs, and defer to `contrib/guix`.

📝 Qoutip opened a pull request: "See text - *"
(https://github.com/bitcoin/bitcoin/pull/28004)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "See text - *"
(https://github.com/bitcoin/bitcoin/pull/28004)

📝 fanquake locked a pull request: "See text - *"
(https://github.com/bitcoin/bitcoin/pull/28004)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

⚠️ john-moffett opened an issue: "Command line options after any non-dash arguments are silently ignored"
(https://github.com/bitcoin-core/gui/issues/741)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

For example, invoking `bitcoin-qt -server=1 foo -regtest` on a fresh install will run `mainnet` instead of the expected `regtest`.



### Expected behaviour

I would expect it to do one of the following:

1. Run `regtest`
2. Warn me that my arguments contained a "command" and any subsequent options were ignored
3. Exit with an error message

The problem is that `ArgsManager::ParseP
...

📝 john-moffett opened a pull request: "Exit and show error if unrecognized command line args are present"
(https://github.com/bitcoin-core/gui/pull/742)
Fixes #741

Starting bitcoin-qt with non-dash ("-") arguments causes it to silently ignore any later valid options. For instance, invoking `bitcoin-qt -server=1 foo -regtest` on a fresh install will run `mainnet` instead of `regtest`.

This change makes the client exit with an error message if any such "loose" arguments are encountered. This mirrors how `bitcoind` handles it:

https://github.com/bitcoin/bitcoin/blob/c6287faae4c0e705a9258a340dfcf548906f12af/src/bitcoind.cpp#L127-L132

💬 achow101 commented on pull request "Miniscript: always treat unsatisfiable scripts as insane":
(https://github.com/bitcoin/bitcoin/pull/27997#discussion_r1246889477)
In d4d3fcc16ccae4755604bb3a9a6a45542b231dcc "miniscript: treat all unsatisfiable miniscripts as insane"

Since this is only used negated, I think it would be better to make this `IsSatisfiable`

```suggestion
bool IsSatisfiable() const { return GetStackSize().has_value(); }
```

💬 darosior commented on pull request "Miniscript: always treat unsatisfiable scripts as insane":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1613538377)
This is on purpose, because:
1. Whether a script is satisfiable depends on the material available, whereas we can tell whether a script will never be satisfiable.
2. `IsSatisfiable` already exists and implements the above (needs to be told what challenges are available).

So to avoid the confusion i opted for the negative. But i could make it `MayBeSatisfiable` if that's clearer?
-------- Original Message --------
On Jun 29, 2023, 6:45 PM, Andrew Chow wrote:

> @achow101 commented on this pull r
...

👍 theuni approved a pull request: "contrib: add macOS test for fixup_chains usage"
(https://github.com/bitcoin/bitcoin/pull/27999#pullrequestreview-1505757163)
utACK 7f96638723a08edf4341a2f4c06b2aa41378fcf7.

The patch looks good. At first glance at `git blame` makes it looks like this is setting the wrong var, but when reading in-context I agree it's correct.

Another reasonable option would be to bump ld64, but since this is the last thing we should ever need from it, I agree patching makes more sense.

Also, I checked lld for `no-fixup-chains` support and it works fine:

> /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ --ta
...

💬 jamesob commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1246940264)
Formatting: `if (`

💬 brunoerg commented on pull request "fuzz: call `LookupSubNet` before calling `Ban` with a subnet":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1613586993)
> Could rebase on master to remove export FUZZ_TESTS_CONFIG="--exclude banman" # https://github.com/bitcoin/bitcoin/issues/27924?

Sure.