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

⚠️ Skylerspencer opened an issue: "Stole wallet and paperwork fraude and robbery this ea stolen from my assets with the help of others dates change false tells created I have the original with dates and time you guys did get all of them showing the truth also the Ethereum accounts stole change dates dates I have yoke pocket all the way to now and father dates times contracts you guys are in deeper you gotta keep lieng stilling my assets with the help of Debbie she told you I knew she would tell you about my Bitcoin and you would come I needemore evidence to prove ,y cases thanks. "
(https://github.com/bitcoin-core/gui/issues/743)