💬 galaxycores commented on pull request "bench: Fix WalletMigration benchmark":
(https://github.com/bitcoin/bitcoin/pull/32281#discussion_r2049676386)
(WriteWachOnly;)

💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049680441)
```C++
BOOST_CHECK_EQUAL((FeeFrac{0x7ffffffdfffffffb, 0x7ffffffd}.EvaluateFeeDown(0x7fffffff)), 0x7fffffffffffffff);
```
This works for me in simple Debug mode (with extra parentheses, built without sanitizers, doesn't fail in Release mode).
Before the fix this also fails with:
> cmake -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build -j$(nproc) && time build/bin/test_bitcoin --run_test='feefrac_tests/feefrac_operators'
...
> Running 1 test case...
> zsh: trace trap

💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2814074180)
Tested ACK 5cb1241814b9c541c5e5e8daadbbea595f2960ae

💬 sipa commented on pull request "test: cover invalid codesep positions for signature in taproot":
(https://github.com/bitcoin/bitcoin/pull/32301#issuecomment-2814090563)
ACK 175740991e0081c35f471cd7f1ad067e3e1f978f

💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2814098958)
> If this ends up fixing all issues, should add --usecli to atleast one CI, so we catch regressions?

Yes - it'd be a bit of a waste of a CI job if we end up disabling too many of the tests with `--usecli`, but depending on how many can be fixed (I'm currently still going through all the existing disabled tests) that would make sense to me.

💬 l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2814103775)
The fuzzer still has some problems with `CCoinsViewCache`:
> runtime error: unsigned integer overflow: 0 - 48 cannot be represented in type 'size_t' (aka 'unsigned long')
[15:43:31.228] #0 0x558e624b3787 in CCoinsViewCache::SpendCoin(COutPoint const&, Coin*) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./coins.cpp:133:22
[15:43:31.228]

I'm not yet sure how that's possible, `FetchCoin` should populate the `cachedCoinsUsage` field if it's missing - will try to reproduce it
...

📝 l0rinc converted_to_draft a pull request: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296)
Inspired by https://github.com/bitcoin/bitcoin/pull/32154, the main goal of this PR is to reenable sanitizer checks for `serialize.h` (last commit)

💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049755996)
Oops, fixed

💬 ariard commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r2049770396)
Thanks for the `bench.cpp`, I had a look on `benchmark_sha256` and `benchmark_signature_validation`.

Though here few more thoughts on the 1-bit cost from the view of a DoSing peer.

Currently, we have 2 caches the signature cache and the `m_script_execution_cache`.

If a script has already an entry at `CheckInputScripts()`, bitcoin core returns true:
https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2185

If there is already a cache entry, and the script includes signa
...

💬 ariard commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2814239023)
> I think it would be valuable to have CTV rather as a taproot op_success.

So I’ve re-read the current version of BIP119 as available in the BIP repository, there is a section about the usage of OP_NOP (`NOP-Default and Recommended Standardness Rules`), however it doesn’t say among the NOP upgradable path and the current upgrade path available with Taproot OP_SUCCESS or an upgrade of the tapscript version.

I’m aware that CTV was designed and drafted for standard, before Taproot got activat
...

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050006636)
You could reframe the comment to explain why this is significant:

```
# Wallet created using relative path won't reside in the default wallet dir,
# hence it should not appear in listwalletdir results.
```

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050010952)
Can we add more stricter checks on the transaction received here.

For example:
```
tx = wallet.gettransaction(txid)
assert_equal(tx["amount"], 1)
assert_equal(tx["confirmations"], 1)
```
This gives you tighter guarantees and helps with debugging if the test fails.

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050018648)
We should first validate that file is present at the specified location and accessible and then only try to open and validate other things

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050015403)
Similar to previous test and all other places, can we fail gracefully with more informative message:
```
assert_equal(..., ..., "Test requires that both wallet paths have the same relative path to the common parent")
```

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050007265)
If this assumption ever fails (say due to environment setup differences), the test will break early.
Consider failing gracefully with a more informative message:

```
assert_equal(..., ..., "Relative paths from wallet dirs to common parent must be identical for this test.")
```

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050013797)
Nit: May be a small comment with reason would help here:
```
# No 'failed_watchonly' directory should exist because migration cleanup should be complete
```

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050019504)
**Minor / Nit:** A separate helper function could be introduced to validate it's BDB for better readability.
Example:
```
def is_bdb_wallet(filepath):
with open(filepath, "rb") as f:
data = f.read(16)
_, _, magic = struct.unpack("QII", data)
return magic == BTREE_MAGIC
```

and then:
```
assert is_bdb_wallet(datfile)
```

💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2814523446)
Concept ACK

💬 maflcko commented on issue "ci: failure in Windows cross-test":
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2814631853)
The error message is a bit confusing, because the benchmarks should be a single process? Thus, it seems odd that another process was accessing the wallet file.

I don't have Windows to debug this, so I guess it is best to temporarily revert. A follow-up can fix the underlying issue and enable the CI check again.

📝 maflcko opened a pull request: "ci: Temporarily revert "Drop bench -priority-level in win cross CI""
(https://github.com/bitcoin/bitcoin/pull/32302)
This reverts commit 27f11217ca63e0f8f78f14db139150052dcd9962.

The commit was nice and useful. However, CI doesn't pass, see https://github.com/bitcoin/bitcoin/issues/32291. Temporarily revert it, so that it can be enabled again along with the issue fixed.