Skip to content

Binary trie #89

Merged
matkt merged 25 commits intobesu-eth:mainfrom
matkt:feature/bin-trie-fix
Oct 27, 2025
Merged

Binary trie #89
matkt merged 25 commits intobesu-eth:mainfrom
matkt:feature/bin-trie-fix

Conversation

@matkt
Copy link
Copy Markdown
Contributor

@matkt matkt commented Aug 14, 2025

PR description

Binary trie implementation by @thomas-quadratic with some fixes by @matkt

Fixed Issue(s)

thomas-quadratic and others added 20 commits June 13, 2025 09:44
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
@github-actions
Copy link
Copy Markdown

  • I thought about the changelog.

@matkt matkt requested a review from garyschulte August 14, 2025 11:50
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Copy link
Copy Markdown

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

LGTM, some thoughts. Can approve to expedite

/**
* The Hex+ string representation of the BitSequence.
*
* @return A byte array representation of the node.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit. string representation of hex bytes of the BitSequence

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, made a change.

*/
@Override
public BytesPackedBitSequence add(int suffix, int width) {
// TODO: modify this.
Copy link
Copy Markdown

@garyschulte garyschulte Aug 14, 2025

Choose a reason for hiding this comment

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

modify how, by using width to mask some of the suffix?

public class BytesBitSequence extends BitSequence<BytesBitSequence> {
private static final BytesBitSequenceFactory FACTORY = new BytesBitSequenceFactory();

public final byte[] data;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

how about java.util.BitSet ? would be more memory efficient

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought about BitSet, or similarly implementing flags with integers. However, there is a primary concern here: concatenating 2 sequences is a frequent operation. Not sure if it would be easy/readable/compute-efficient to concatenate BitSets, as opposed to boolean[].
So the idea: benchmark with the simple boolean[] solution first and see if there could be value to memory efficiency. Try other implementations if necessary like BitSets.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

boolean[] is a great idea to keep the existing implementation and be memory efficient 👍

sb.append("."); // There is at least 1 remaining bit
}
for (int j = i; j < bitLength; j++) {
sb.append(get(i) ? '1' : '0');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be j here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, thx

@matkt
Copy link
Copy Markdown
Contributor Author

matkt commented Aug 15, 2025

@thomas-quadratic if you can take a look

@thomas-quadratic
Copy link
Copy Markdown
Contributor

Ok, I merged @matkt fixes, and some docs.

matkt added 4 commits August 27, 2025 10:30
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
@matkt
Copy link
Copy Markdown
Contributor Author

matkt commented Oct 21, 2025

Fixed another issue related to SHA256 on this PR. I think we should be good to check the last comment and merge this PR @thomas-quadratic @garyschulte

Copy link
Copy Markdown

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

latest commit LGTM

@matkt matkt merged commit c78f170 into besu-eth:main Oct 27, 2025
5 checks passed
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.

3 participants