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

💬 brunoerg commented on pull request "fuzz: rule-out too deep derivation paths in descriptor parsing targets":
(https://github.com/bitcoin/bitcoin/pull/28832#discussion_r1394252904)
Correct me if I'm wrong, but wouldn't `HasDeepDerivPath` avoid descriptors like `pkh([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)`? Is this intended?

📝 dergoegge opened a pull request: "fuzz: Delete wallet_notifications"
(https://github.com/bitcoin/bitcoin/pull/28882)
This harness is not doing much since it is incredibly slow. It's probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.

On oss-fuzz for example, it never even starts fuzzing because it times-out while running through the existing corpus:

```
Component revisions (build r202311100604):
Bitcoin-core: b3898e946cf81e2e7b573e1c5204bd29af2feecd
Botan: 201cfe586e6d529360fbcde6f216f1da0c3db48f
Cryptofuzz: 537263836ea
...

💬 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-1812610712)
Currently `GetOpCode()` will return `OP_UNKNOWN` for unknown opcodes which seems reasonable. Dropping the `OP_` would appear in the asm as `UNKNOWN`.

As for undecodable bytes, if we did want a keyword, perhaps something more explicit like `UNDECODABLE<...>` would be better? We are essentially using what I would expect `RAW` to denote for > 5 byte minimal pushes if we go with this:

> Any other minimal pushes (so direct push <= 75 bytes, PUSHDATA1 > 75 bytes, PUSHDATA2 > 255 bytes) become <.
...

💬 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-1812617163)
> Currently GetOpCode() will return OP_UNKNOWN for unknown opcodes which seems reasonable.

That's trivially ambiguous, as *all* unknown opcodes are mapped to the same thing.

> As for undecodable bytes, if we did want a keyword, perhaps something more explicit like UNDECODABLE<...> would be better?

Maybe, but I wouldn't use `<...>` because it's not a push.

> We are essentially using what I would expect RAW to denote for > 5 byte minimal pushes if we go with this:

No, *any* pushes b
...

💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812622711)
> This harness is not doing much since it is incredibly slow. It's probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.

I think it was added as a regression fuzz test, so it can find at least that one regression. I am running it on my servers and I agree it should be improved.

💬 mzumsande commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812702510)
> Note also, that item 7 in the PR description involves a regression in v26.x.

I don't think that this can be called a regression:
* There is nothing new about the root problem (making automatic connections to addnode peers in rare cases) in v26.
* It can happen that such a peer could now be protected as the only one of its network, but to determine if it's a regression this needs to be compared with the status before.
* Before we did protection-by-network, we would only ever get into the
...