Skip to content

crypto: Fix MuHash3072 division by itself - #453

Open
2rdzy wants to merge 1 commit into
bitcoinknots:29.x-knotsfrom
2rdzy:fix-muhash-self-division
Open

2rdzy wants to merge 1 commit into
bitcoinknots:29.x-knotsfrom
2rdzy:fix-muhash-self-division

Conversation

@2rdzy

@2rdzy 2rdzy commented Oct 8, 2026

Copy link
Copy Markdown

Summary

Backport of bitcoin#36286. MuHash3072::operator/= gives a wrong result when the divisor is the object itself (x /= x). Base: 58398baf33 (upstream/29.x-knots).

Problem

src/crypto/muhash.cpp:569: the first Multiply changes m_numerator, and the second reads div.m_numerator, which is the same object. For x = N/D the result is N·D / (D·N·D) = 1/D instead of 1. It is only correct when D = 1.

Reach: production code (coinstats, coinstatsindex, gettxoutsetinfo) only calls Insert and Remove. Nothing in the node calls /= or *=. This is a latent library bug, found by tests and fuzzing, with no effect on UTXO set hashes.

Reproduction

Add the test hunks from this PR without the muhash.cpp change, then run build/bin/test_bitcoin --run_test=crypto_tests/muhash_tests.

Fuzz: configure with -DBUILD_FOR_FUZZING=ON, build fuzz, run FUZZ=muhash build_ff/bin/fuzz <files>. I used 300 random seed files because no qa-assets corpus was available, and built with gcc, not libFuzzer.

Before

crypto_tests.cpp(1260): error: in "crypto_tests/muhash_tests": check out == out2 has failed
*** 10 failures are detected
fuzz: ./test/fuzz/muhash.cpp:228: Assertion `out == out2' failed.   (input seeds/s206)

The original fuzz target (without the added Remove) passes the same 300 files.

Change

+    const Num3072 div_numerator{div.m_numerator}; // div may alias *this
     m_numerator.Multiply(div.m_denominator);
-    m_denominator.Multiply(div.m_numerator);
+    m_denominator.Multiply(div_numerator);

Plus Core's test changes in src/test/crypto_tests.cpp (+6) and src/test/fuzz/muhash.cpp (+3). They applied unchanged from Core.

After

muhash_tests passes. Fuzz: muhash: succeeded against 300 files.

Test results

  • Commit tested: 58398baf33 plus this change.
  • Build: Release, tests, GUI, wallet, SQLite, ZMQ and USDT all on.
  • ctest: 147/147 passed.
  • Functional --extended: 359 tests, 295 passed, 64 skipped, 0 failed.
  • Skips: BDB/legacy-wallet and environment-dependent tests. The same 64 skipped before and after the change.

Caveats

  • Fuzzing used random seeds with gcc and no coverage guidance.
  • The first baseline functional run had 2 failures from port collisions with a concurrent run. They passed when rerun alone on the same base.
  • The change does not touch consensus code.

MuHash3072::operator/= multiplies the numerator by div's denominator and
then the denominator by div's numerator. If div aliases *this, the first
step has already changed m_numerator, so the second step uses the wrong
value and x /= x does not give the empty set.

Copy div's numerator before the first multiplication. Extend the unit
test and the fuzz target (which only divided an object whose denominator
was still 1) to cover self-division with a non-trivial denominator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant