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

💬 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-1812500265)
@MatthewLM Right, any single numeric push of even number of digits inside `<>` would be ambiguous. But also, scripts consisting of a single push are generally useless as a redeemscript (they'd be push only), so I'm not sure it's an issue. If you actually encounter one of those, just don't do recursive decoding.

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1812506504)
A fresh one:
[clusterfuzz-testcase-mini_miner-5494419377487872.bin.txt](https://github.com/bitcoin/bitcoin/files/13365006/clusterfuzz-testcase-mini_miner-5494419377487872.bin.txt)


💬 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-1812532463)
One more thought: if the format we end up is unambiguous, there could also be a function/RPC/tool to convert asm back to script (and we can have fuzz tests that the roundtripping encoding+decoding is exact!). If so, we can probably permit a few things in the decoding that the encoder might not (yet) support:
* Optionally `OP_` prefixing opcodes (and using aliases like `TRUE` or `OP_TRUE` for `OP_1`).
* Using `0x...` for BE hex encoded numbers in all places where a decimal number is supported (
...

💬 willcl-ark 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-1812534458)
I like where this is going now. Thanks for the conversation.

I've written a branch implementing this [previous version](https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1810204432) as I wanted to see what the changes would look like (although I'm not convinced I'm handling `OP_1NEGATE` properly yet, even though tests are passing...)

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

📝 fanquake opened a pull request: "doc: remove mention of missing bdb being a configure error"
(https://github.com/bitcoin/bitcoin/pull/28881)
This is no-longer the case, unless you're passing additional flags, which is not the case in this example.

🤔 BrandonOdiwuor reviewed a pull request: "test: Use test framework utils in functional tests"
(https://github.com/bitcoin/bitcoin/pull/28528#pullrequestreview-1732051432)
Concept ACK

This PR improves functional tests (readability) by using/introducing complimentary util functions to `assert` ie `assert_equal`, `assert_not_equal`, `assert_less_than`, `assert_less_than_or_equal`

💬 maflcko commented on pull request "doc: remove mention of missing bdb being a configure error":
(https://github.com/bitcoin/bitcoin/pull/28881#issuecomment-1812557861)
lgtm ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc

💬 ajtowns 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-1812570217)
> * 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 _231_ and _239-1_, is just encoded in decimal directly (no `<>` or anything).

`-2**31` and `2**39-1` presumably? (original lacks the negative sign, and github can't quote superscript correctly apparently)

> * Any other minimal pushes (so direct push <= 75 bytes, PUSHDATA1 > 75 bytes, PUSHDATA2 > 255 bytes) become `<...>` with ... the hex-enco
...

💬 apoelstra commented on issue "`libbitcoinconsensus.a` is unusable":
(https://github.com/bitcoin/bitcoin/issues/28779#issuecomment-1812573445)
Thanks for the heads up. We already do something weird in rust-bitcoin to avoid bundling two copies of libsecp (or at least, we've talked a lot about this) so we might avoid this error. But if it pops up the next time we're updating rust-bitcoinconsensus I will remember this issue!

👍 theuni approved a pull request: "Use serialization parameters for CTransaction"
(https://github.com/bitcoin/bitcoin/pull/28438#pullrequestreview-1732093948)
Thanks for addressing my feedback. Sounds like you already have plans for the things I complained about, so no reason to hold this up.

ACK a0c254c13a3ef21e257cca3493446c632b636b15

💬 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-1812587025)
@ajtowns Right on all 3 counts. Instead of limiting the integer range, maybe just "all minimal pushes of up to 5 bytes, which push a minimally-encoded 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-1812596092)
One more question: what to do with unknown opcodes, and undecodable bytes? The current `FormatScript` just turns it into hex, which is ambiguous.

Suggestion: use `RAW(...)` with "..." the hex in wire byte order for those. I'd prefer not to use `<...>` because I associate that with "pushing", which wouldn't be correct here. I'm not too happy with having a special keyword in the syntax just for that, suggestions welcome.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1812597408)
`8f21993a06...2386ef0a54`: rebase due to conflicts and drop the newly added fuzz test.

@dergoegge, thanks for looking into this and testing it. The reason for the low coverage is, I guess, that the fuzzer has a hard time generating a valid or "interesting" data to push to the socket. Same as the reason for https://github.com/bitcoin/bitcoin/pull/28692. According to @maflcko's advice https://github.com/bitcoin/bitcoin/pull/21387#discussion_r893637770 a dictionary or a custom mutator could reso
...