Skip to content

feat: some fixes#105

Open
SWvheerden wants to merge 3 commits intotari-project:mainfrom
SWvheerden:sw_fix_scanning
Open

feat: some fixes#105
SWvheerden wants to merge 3 commits intotari-project:mainfrom
SWvheerden:sw_fix_scanning

Conversation

@SWvheerden
Copy link
Contributor

Description

Fix scan beyond tip
Checks the tip if the start height is set, so that we dont scan beyond the tip
Fix speed of display transaction
Fixes the display transaction construction to use branch and bound to find the inputs

fix scan beyond tip
fix speed of display transaction
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces important fixes to enhance the robustness and efficiency of the system. It prevents blockchain scanning operations from attempting to scan beyond the current tip, ensuring data consistency and avoiding unnecessary processing. Additionally, it significantly optimizes the construction of displayed transactions by improving the input selection algorithm, leading to faster and more efficient transaction processing.

Highlights

  • Prevent scanning beyond blockchain tip: Implemented a check in the HTTP scanner to ensure that the requested 'start_height' for a scan does not exceed the current blockchain tip, returning empty results if it does.
  • Optimize displayed transaction construction: Improved the performance of displayed transaction construction by sorting potential inputs by value in reverse order and refining the branch and bound search algorithm for matching inputs.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to improve the performance of finding matching inputs for transactions by implementing a branch and bound algorithm. While the refactoring to a backtracking approach is a good direction for performance, the new implementation contains a critical bug in its pruning logic that can cause it to miss valid solutions. I've provided a comment with a fix for this issue. The other changes are minor improvements and style fixes.

@SWvheerden
Copy link
Contributor Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several fixes, including preventing scanning beyond the tip height and improving the performance of displayed transaction construction. The changes to the iter_search_for_matching_inputs function in processor.rs are particularly notable, as they refactor the input selection algorithm to use a more efficient branch-and-bound approach, avoiding unnecessary vector cloning in recursive calls. Additionally, the initialization of selected_inputs with a pre-allocated capacity is a good optimization.

// we need to create a new display tx for this input
new_debit.push((balance_change.clone(), output.clone()));
}
new_debit.sort_by_key(|a| a.1.output.value());
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The new_debit vector is sorted in ascending order by output value. For a branch-and-bound algorithm like iter_search_for_matching_inputs, sorting inputs in descending order of value can often lead to faster convergence by pruning branches earlier. Since std::cmp::Reverse is imported but not used, consider sorting in reverse order to potentially improve performance. If iter_search_for_matching_inputs is not on a critical path and is used infrequently, this optimization could be deferred as per repository guidelines on performance trade-offs.

Suggested change
new_debit.sort_by_key(|a| a.1.output.value());
new_debit.sort_by_key(|a| Reverse(a.1.output.value()));
References
  1. Performance optimizations for functions not on a critical path and used infrequently can be deferred, noting the trade-off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gemini-code-assist
because we want search for explicit matches, if we sort the branch ascending order, when we get to the actual search algorhtim, we can do this:

let new_total = amount_already_selected + input_value;
            if new_total > amount_sent {
                return false;
            }

there by pruning the branch.
If we do decending order, we cannot do this and we have to finish searching that entire order

Copy link
Contributor

Choose a reason for hiding this comment

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

You make a very good point. My suggestion was based on a general heuristic for branch-and-bound algorithms, where sorting in descending order can often lead to faster convergence by finding a solution quicker or pruning branches that quickly exceed the target. However, for your specific implementation, where you are looking for an exact match and have the pruning condition if new_total > amount_sent { return false; }, sorting the inputs in ascending order is indeed more effective. This allows you to prune branches as soon as the sum exceeds amount_sent, knowing that any subsequent (larger) inputs would also exceed the target. Thank you for clarifying the specific logic of iter_search_for_matching_inputs.

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