Skip to content

Conversation

@shruggr
Copy link
Collaborator

@shruggr shruggr commented Jul 17, 2025

Description

This PR fixes multiple BEEF-related issues:

  1. Non-deterministic BEEF validation ([BUG] BEEF IsValid/Verify algorithm is unstable - gives different results for the same input BEEF #211)
  2. BEEF parsing panic with "no leaves at height" error ([BUG]There are no leaves at height: ... #96)
  3. Incorrect validation logic for transactions in BUMPs
  4. Confusing method name SortTxs

Root Cause Analysis

Issue #211: The validation logic was not properly handling transaction validation order, causing non-deterministic results.

Issue #96: The SDK was being too strict during BEEF parsing, panicking when transactions didn't have merkle paths or source transactions. This is valid BEEF data that should be handled gracefully.

Validation Logic: The validation wasn't properly checking if transactions appear in BUMPs (have proof) for all DataFormat types.

Solutions

  1. Fixed validation order by ensuring transactions with merkle paths are added to the valid set before validating dependent transactions

  2. Removed panic during parsing - transactions without merkle paths or source transactions are now handled gracefully

  3. Refactored validation logic:

    • Renamed SortTxs() to ValidateTransactions() for clarity
    • Added proper validation for TxIDOnly and RawTx formats by checking if they appear in BUMPs
    • Added verification that RawTxAndBumpIndex has accurate bump index
    • Returns a structured ValidationResult with categorized transaction lists
  4. Fixed test assumptions that incorrectly expected all input transactions to be present in BEEF

  5. Performance optimization:

    • Refactored BEEF implementation to use chainhash.Hash directly as map keys instead of string conversions
    • Added *ByHash method variants to avoid unnecessary hash/string conversions
    • Updated collectAncestors to return []chainhash.Hash instead of []string

Additional Fixes

Testing

Changes

  • Modified verifyValid() to use new ValidateTransactions() method
  • Added ValidateTransactions() method with comprehensive validation logic
  • Removed panic in readAllTransactions() for missing source transactions
  • Updated all test references from SortTxs to ValidateTransactions
  • Fixed test in TestBeefGetValidTxids to handle transactions with merkle paths correctly
  • Refactored internal BEEF structures to use chainhash.Hash for better performance

Fixes #211
Fixes #96
Fixes #202

shruggr added 3 commits July 17, 2025 14:08
- Add transactions with merkle paths to valid set before validation
- Use iterative validation for proper dependency ordering
- Ensure deterministic validation results

Fixes #211
Use 'go get' instead of 'go install' for adding the SDK as a dependency.
The go-sdk is a library package, not an executable.

Fixes #202
@shruggr shruggr requested a review from rohenaz July 17, 2025 18:24
@kuba-4chain
Copy link

kuba-4chain commented Jul 17, 2025

Hey, I just added a similar fix here:
#213

I did not see you've already pushed the fix, you beat me to it 😄 I'll close my PR as a duplicate.

There's just one thing, I think that the b.SortTxs() function at the beginning of the verifyValid method is unnecessary and could be removed, as it doesn't sort in-place, if I understand this correctly.

Other than that, thanks for the fix!

@kuba-4chain kuba-4chain mentioned this pull request Jul 17, 2025
10 tasks
@shruggr shruggr changed the title Fix BEEF validation stability issue (#211) Fix BEEF validation issues and refactor validation logic (#211, #96) Jul 17, 2025
shruggr added 2 commits July 17, 2025 18:16
- Fix validation to check BUMPs properly
- Rename SortTxs to ValidateTransactions
- Verify bump indices for RawTxAndBumpIndex
- Fix test assumptions
- Use chainhash.Hash directly as map keys for better performance
- Add *ByHash method variants to avoid unnecessary conversions
- Update collectAncestors to return hash slice
- Fix all tests to work with new hash-based implementation
shruggr and others added 4 commits July 18, 2025 11:17
Co-authored-by: chris-4chain <152964795+chris-4chain@users.noreply.github.com>
Co-authored-by: chris-4chain <152964795+chris-4chain@users.noreply.github.com>
Copy link
Collaborator

@rohenaz rohenaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@shruggr shruggr merged commit 50b99da into master Jul 21, 2025
5 of 6 checks passed
@shruggr shruggr deleted the fix/beef-validation-stability branch July 21, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants