💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1812264645)
Concept ACK.

💬 ajtowns commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1394031188)
Really, we're already wasting 16 bytes due to `vin` and `vout` supporting `capacity() != size()` despite both being `const` and never needing to be resized.

💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#discussion_r1394051068)
The SDK version in a Guix built bitcoind is still being set to 11.0:
```bash
Load command 11
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 11.0
sdk 11.0
ntools 1
tool 3
version 711.0
```

👍 hebasto approved a pull request: "build: Windows SSP roundup"
(https://github.com/bitcoin/bitcoin/pull/28461#pullrequestreview-1731808847)
ACK f95af98128f17002bf137a48441167020f3ef9bb, I've verified binaries from `bitcoin-f95af98128f1-win64.zip` on Windows 11 Pro 23H2.

However, the penultimate commit works just fine as well. The `diffoscope` show minor changes in binaries.

So do we actually need the last commit at all?

💬 fanquake commented on pull request "build: Windows SSP roundup":
(https://github.com/bitcoin/bitcoin/pull/28461#issuecomment-1812317916)
> So do we actually need the last commit at all?

The point is to explicitly enable the hardening option. This is the same as we do for the Linux GCC.

💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28851#issuecomment-1812341926)
Could be closed in favour of https://github.com/bitcoin/bitcoin/pull/28880.

💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1812354204)
> this PR also unblocks #28622 (C++20)

... due to [this](https://github.com/llvm/llvm-project/commit/c8e2dd8c6f490b68e41fe663b44535a8a21dfeab) commit that fixes Qt package build.

💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1812374637)
From Guix build logs it follows that Clang 15.0.7 is used. Not Clang 17?

💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#discussion_r1394092853)
> Looks like we'll have to add a -Wl,-platform_version into ldflags here.

Should be resolved now.

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1812389164)
> From Guix build logs it follows that Clang 15.0.7 is used. Not Clang 17?

I don't think so? If that was the case the Guix builds in #28622 would be failing to build Qt? They are currently working with the most recent push, and addition of `-platform_version`.

💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1812417405)
> > From Guix build logs it follows that Clang 15.0.7 is used. Not Clang 17?
>
> I don't think so?

Right. There was a wrong build setup on my side. Sorry for the noise.

💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1812432364)
My Guix builds:
```
x86_64
34e5ff49486a0777be8c0cba514a0de0e010573ada67c4e7fc0d5f46b4489ac2 guix-build-1a9d3eb54ba0/output/arm64-apple-darwin/SHA256SUMS.part
c0b4a1b61a9b65c1765fbf33b1c0ea53b1f6aeb5efd5e3a2051f193f34f345fd guix-build-1a9d3eb54ba0/output/arm64-apple-darwin/bitcoin-1a9d3eb54ba0-arm64-apple-darwin-unsigned.tar.gz
911b8fa0cdc61195e96c479b5173b333f9efa647cbc71ef499284905ef3e1e1b guix-build-1a9d3eb54ba0/output/arm64-apple-darwin/bitcoin-1a9d3eb54ba0-arm64-apple-darwin-unsigned
...

💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1812434474)
Guix hashes for `arm64-apple-darwin` differs across the build platforms, unfortunately.

💬 MatthewLM commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812455750)
For 32-bit signed integers, the most significant bit is negative, but that's not true for CLTV and CSV which uses 5 bytes. So a push data could represent a negative 32-bit signed integer, or a positive 40-bit signed integer if I understand the `CScriptNum` without looking more closely at it.

💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812458710)
@MatthewLM I don't think that's correct. What's special in CSV and CLTV is that they permit numbers whose encoding is up to 5 bytes (rather than the math opcodes which only support encodings up to 4 bytes on input). But the encoding algorithm is the same: the top bit is the sign.

💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1812459716)
Guix Build (aarch64):
```bash
3b9d19ed8a1d4429ec5228afc9d8d9cddb76dfaa7a403edca7ab9b24751f22ef guix-build-cf8353a9cf9c/output/arm64-apple-darwin/SHA256SUMS.part
7371c9750834e778fff18d3ea2b8e98cec90fdcf3219e0a1e6d13841c6877d70 guix-build-cf8353a9cf9c/output/arm64-apple-darwin/bitcoin-cf8353a9cf9c-arm64-apple-darwin-unsigned.tar.gz
d8e271154e572f716b853cf525b9c83bc9b7ca2a07f8862a8eb45a327f6193e3 guix-build-cf8353a9cf9c/output/arm64-apple-darwin/bitcoin-cf8353a9cf9c-arm64-apple-darwin-unsign
...

🤔 ismaelsadeeq reviewed a pull request: "Bugfix: Package relay / bytespersigop checks"
(https://github.com/bitcoin/bitcoin/pull/28345#pullrequestreview-1731941173)
Concept ACK

💬 MatthewLM commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812462943)
@sipa I'll have to look at the code. I image it is two's complement? Then the negative bit will be different for each type of integer meaning that a 4 byte pushdata that would encode a negative 32-bit signed integer could also be interpreted as a positive 40-bit signed integer.

💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812477898)
@ajtowns Ok, iterating on that, new proposal:

* Any minimally-encoded number, using a minimal push (`OP_n`, or direct push for numbers that cannot be encoded using OP_n) of an integer between *2<sup>31</sup>* and *2<sup>39</sup>-1*, is just encoded in decimal directly (no `<>` or anything).
* `OP_` prefix dropped in general
* Drop sighash type.
* Any other minimal pushes (so direct push <= 75 bytes, PUSHDATA1 > 75 bytes, PUSHDATA2 > 255 bytes) become `<...>` with ... the hex-encoding (in b
...

💬 MatthewLM commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812490008)
@sipa Sorry, you are right. I looked at the code again. Bitcoin doesn't use two's complement for this (I have to keep in mind that Bitcoin has a lot of quirks). It uses a sign bit that negates the other bits, and this sign bit depends on the size of the push data (whatever the last byte is), so there is no ambiguity.

Therefore I think minimally pushed numbers ought to be displayed as decimal for readability reasons as you now suggest.

> * Inside `<...>` instead of having hex data, it's als
...