💬 josibake commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1931693124)
ACK https://github.com/bitcoin/bitcoin/pull/29112/commits/cfcb9b1ecf9be5267487713fa1e112ca09a1ae55

Looks good, nothing to add. Happy to reACK if you end up taking ryanofsky's suggestions.

💬 hebasto commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1931724034)
I agree that all required headers must be included explicitly without relying on indirect inclusions via other headers.

💬 maflcko commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-1931758957)
Ref: https://github.com/bitcoin/bitcoin/blame/v26.0/src/wallet/spend.cpp#L1223-L1227

💬 maflcko commented on pull request "test: Fix CPartialMerkleTree.nTransactions signedness":
(https://github.com/bitcoin/bitcoin/pull/29363#issuecomment-1931772226)
rfm?

📝 maflcko opened a pull request: "test: Fix utxo set hash serialisation signedness"
(https://github.com/bitcoin/bitcoin/pull/29399)
It is unsigned in Bitcoin Core, so the tests should match it:

https://github.com/bitcoin/bitcoin/blob/5b8990a1f3c49b0b02b7383c69e95320acbda13e/src/kernel/coinstats.cpp#L54


Large positive values for the block height are impossible to hit in tests, but it still seems fine to fix this.

The bug was introduced when the code was written in 6ccc8fc067bf516cda7bc5d7d721945be5ac2003.

(Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters)

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1481302085)
fixed

📝 maflcko opened a pull request: "test: Fix SegwitV0SignatureMsg nLockTime signedness"
(https://github.com/bitcoin/bitcoin/pull/29400)
It is unsigned in Bitcoin Core, so the tests should match it:

https://github.com/bitcoin/bitcoin/blob/5b8990a1f3c49b0b02b7383c69e95320acbda13e/src/script/interpreter.cpp#L1611

The bug was introduced when the code was written in 330b0f31ee5719d94f9e52dfc83c5d82168241f9.

(Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters)

💬 maflcko commented on pull request "test: Fix SegwitV0SignatureMsg nLockTime signedness":
(https://github.com/bitcoin/bitcoin/pull/29400#issuecomment-1931820988)
This can be tested by passing a transaction with a large locktime and then observing the error:

```
struct.error: 'i' format requires -2147483648 <= number <= 2147483647
```

💬 paplorinc commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#discussion_r1481313446)
I'm just looking around, I'm not a Bitcoin core expert.
My understanding is that this is unsigned little endian is `<I`, wouldn't it make sense to change as little as possible here:
```suggestion
data += struct.pack("<I", height * 2 + coinbase)
```
or if we want to mimic the C++ code:
```suggestion
data = struct.pack('<I', (height << 1) + coinbase)
```

💬 paplorinc commented on pull request "test: Fix SegwitV0SignatureMsg nLockTime signedness":
(https://github.com/bitcoin/bitcoin/pull/29400#discussion_r1481315962)
Could we keep the format?
```suggestion
struct.pack("<I", txTo.nLockTime)
```

In the instructions you've mentioned it's reproducible - can you add a test that fails before this change, but passes after?

💬 paplorinc commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#discussion_r1481317057)
if possible, can you cover it with a test that reproduces the error before this fix - and passes after it?

💬 maflcko commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#discussion_r1481319871)
> wouldn't it make sense to change as little as possible here:

No. `struct.pack` is brittle and confusing. Just look at all the bugs: https://github.com/bitcoin/bitcoin/pull/29400, https://github.com/bitcoin/bitcoin/pull/29399, https://github.com/bitcoin/bitcoin/pull/29363, fa3886b7c69cbbe564478f30bb2c35e9e6b1cffa, ...

Thus, `struct.pack` should be removed from all tests to avoid more bugs in the future.

💬 maflcko commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#discussion_r1481320201)
No, Large positive values for the block height are impossible to hit in tests, but it still seems fine to fix this.

💬 maflcko commented on pull request "test: Fix SegwitV0SignatureMsg nLockTime signedness":
(https://github.com/bitcoin/bitcoin/pull/29400#discussion_r1481330149)
Yes, you can try this with the following diff:

```
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index d316c4b602..708e4d7e8f 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -1663,6 +1663,7 @@ class SegWitTest(BitcoinTestFramework):
pubkeyhash = hash160(pubkey)
script_pkh = key_to_p2wpkh_script(pubkey)
tx = CTransaction()
+ tx.nLockTime = 0xffffffff
tx.vin.append(CTxIn(COutPoint
...

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1481349496)
Is this needed?

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1481350393)
nit: Could use f-string instead of `%` when touching this line?

📝 maflcko opened a pull request: " test: Remove struct.pack from almost all places "
(https://github.com/bitcoin/bitcoin/pull/29401)
`struct.pack` has many issues:

* The format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.

This lead to many test bugs, which weren't hit, which is fine, but still confusing.

Fix all issues by using the built-in `int` helpers `to_bytes` via a scripted diff.

Review notes:

* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messa
...

📝 maflcko converted_to_draft a pull request: "test: Remove struct.pack from almost all places"
(https://github.com/bitcoin/bitcoin/pull/29401)
`struct.pack` has many issues:

* The format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.

This lead to many test bugs, which weren't hit, which is fine, but still confusing.

Fix all issues by using the built-in `int` helpers `to_bytes` via a scripted diff.

Review notes:

* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messa
...

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1481381389)
fixed

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1481382045)
fixed