💬 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

💬 MarcoFalke commented on pull request "Exit and show error if unrecognized command line args are present":
(https://github.com/bitcoin-core/gui/pull/742#issuecomment-1613669507)
I always wondered why the code wasn't the same. Was this needed for payment requests, when they were linked to be opened by the gui application?

💬 hebasto commented on pull request "contrib: add macOS test for fixup_chains usage":
(https://github.com/bitcoin/bitcoin/pull/27999#issuecomment-1613675709)
Concept ACK.

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

So, what is the plan for bumping ld64 in the future?

💬 fanquake commented on pull request "contrib: add macOS test for fixup_chains usage":
(https://github.com/bitcoin/bitcoin/pull/27999#issuecomment-1613679524)
> So, what is the plan for bumping ld64 in the future?

Use LLD.

💬 hebasto commented on pull request "contrib: add macOS test for fixup_chains usage":
(https://github.com/bitcoin/bitcoin/pull/27999#issuecomment-1613687508)
> > So, what is the plan for bumping ld64 in the future?
>
> Use LLD.

Ah, I can see that the first commit has been pulled from https://github.com/bitcoin/bitcoin/pull/21778.

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1247025846)
Up to C++11, my understanding was to avoid `auto` with uniform (braced) initialization because in some cases it could have unexpected results. Since C++17, that was mostly fixed and the CPP Guideline is to [prefer the {}-initializer syntax](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax) as its rules are simpler, more general, less ambiguous, and safer. See also https://ianyepan.github.io/posts/cpp-uniform-initialization/. Though it didn't matte
...