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

📝 MarcoFalke opened a pull request: "refactor: Open file in FileCommit "
(https://github.com/bitcoin/bitcoin/pull/28006)
This improves `FileCommit`: The patch removes the requirement from the call site to open a raw C-style FILE pointer. Now the call site can use any style of file IO.

There are many other smaller improvements, which are explained in each commit.

💬 stratospher commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1246962984)
true! added.

💬 stratospher commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1246963378)
makes sense to have test vectors. i've included them.

💬 stratospher commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1246963499)
done.

💬 stratospher commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#issuecomment-1613603919)
Thanks @sipa, @theStack! I've updated to include ellswift test vectors from BIP 324 + addressed review comments.

💬 theuni commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#issuecomment-1613609265)
Thanks! Now mind updating to use `Assert()`? :)

🤔 mzumsande reviewed a pull request: "test: Use same timeout for all index sync"
(https://github.com/bitcoin/bitcoin/pull/27988#pullrequestreview-1505846775)
code review ACK fa086248e57b89cc549a090f727c0978082727c0

Bumping the timeout for txindex and blockfilterindex should hopefully fix the intermittent CI fails.

💬 apoelstra commented on pull request "Miniscript: always treat unsatisfiable scripts as insane":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1613616505)
0 is sane according to https://github.com/rust-bitcoin/rust-miniscript/blob/master/src/miniscript/analyzable.rs#L66

If we are going to change the definition can we please have some reference document somewhere about what "sane" actually means?

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1613651346)
ACK 612ba17fcadc955c142f21f9b1af0b60f0def55f

👍 hebasto approved a pull request: "refactor: remove in-code warning suppression"
(https://github.com/bitcoin/bitcoin/pull/28002#pullrequestreview-1505907503)
ACK 3210f224dbeaf0f9de09d4e83c3e6666128a6717