💬 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

👍 maflcko approved a pull request: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/29387#pullrequestreview-1867666239)
Thanks. lgtm

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1481384059)
```suggestion
subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + [f"--tmpdir={tmpdir}/cache", "--descriptors"])
```

💬 Empact commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1931936133)
Is it not worthwhile to prevent reallocation if we can, even if there is not a data disclosure risk with this allocator?

💬 maflcko commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-1931938143)
I presume the following may be interesting for someone to debug this, if you don't mind to share:

Which wallet settings are you using? (Feerate, etc)
Which RPCs are you using?
Which amounts and settings are you passing to the RPCs? SFFO?
How many wallet coins do you have?
Maybe an exact list with amounts ?

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

💬 azazar commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-1931949888)
bitcoind settings: -limitancestorcount=1000 -txindex=1 -testnet -server

RPCs: sendmany('', $amounts, 0, '', [], true, 1) / getbalance('*', 1) / getbalance('*', 0)

Amounts vary, but quite small always. You can examine some of the last transactions:

https://live.blockcypher.com/btc-testnet/tx/980f2c14c3930d31485b94ae449177dfa8dadeeaba838d5a0a171e039d955dd2
https://live.blockcypher.com/btc-testnet/tx/298d311345c671b6cac8ca58d683789f736de106d69b67fb49cb261dc2b351c1
https://live.blockcyphe
...

💬 maflcko commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1931952848)
> This does seem like an nice and easy solution. Would be good to incorporate in #29331

Not sure if it so easy. On 32-bit platforms, `nTx` currently starts at byte 60, so making it 16-bit will still eat 32-bit with padding. So forcing a 64-bit blob of both values is needed, but that requires changing all code that uses either field. Not sure if worth it for a +-5% memory difference?

💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1481412610)
This test was originally generic and it was moved to sqlite-only because of a bdb bug, see https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1795448339.