Skip to content

flamenco: Add BitVector type #4942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 2, 2025
Merged

flamenco: Add BitVector type #4942

merged 1 commit into from
May 2, 2025

Conversation

cali-jumptrading
Copy link
Contributor

@cali-jumptrading cali-jumptrading commented Apr 30, 2025 •
edited
Loading

Fixes Types Fuzzer mismatch by adding support for a BitVector member type in gen_stubs.py and gen_fuzz.py. We need a BitVector type because there is an invariant check that the length of the bitvector matches the length of the number of bits.

Verified that this fixes the types fuzzer mismatch locally in solfuzz.

@cali-jumptrading cali-jumptrading changed the title Add BitVector type WIP: Add BitVector type Apr 30, 2025
@cali-jumptrading cali-jumptrading changed the title WIP: Add BitVector type WIP: flamenco: Add BitVector type Apr 30, 2025
@cali-jumptrading cali-jumptrading marked this pull request as ready for review April 30, 2025 15:03
@cali-jumptrading cali-jumptrading marked this pull request as draft April 30, 2025 15:05
@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch 2 times, most recently from 301c20b to 59497c3 Compare April 30, 2025 20:46
@cali-jumptrading cali-jumptrading marked this pull request as ready for review April 30, 2025 20:47
@ripatel-fd
Copy link
Contributor

ripatel-fd commented Apr 30, 2025 •
edited by cali-jumptrading
Loading

@cali-jumptrading I think it might be easier to make the bitvec type an opaque type. Then implement it in fd_types_custom. That way, you can just write the bitvec-specific logic in C, and can avoid the gen_stubs.py script. (see fd_flamenco_txn for an example)

@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch from 59497c3 to 0811ced Compare April 30, 2025 21:32
@cali-jumptrading
Copy link
Contributor Author

@cali-jumptrading I think it might be easier to make the bitvec type an opaque type. Then implement it in fd_types_custom. That way, you can just write the bitvec-specific logic in C, and can avoid the gen_stubs.py script. (see fd_flamenco_txn for an example)

I would have to have at least two different definitions of the bitvec type though... because we currently use a u8 bitvec and a u64 bitvec. Unless I used a macro to define the bitvec type as an opaque type...

@cali-jumptrading
Copy link
Contributor Author

@cali-jumptrading I think it might be easier to make the bitvec type an opaque type. Then implement it in fd_types_custom. That way, you can just write the bitvec-specific logic in C, and can avoid the gen_stubs.py script. (see fd_flamenco_txn for an example)

I would have to have at least two different definitions of the bitvec type though... because we currently use a u8 bitvec and a u64 bitvec. Unless I used a macro to define the bitvec type as an opaque type...

From a consistency standpoint, it makes more sense to implement the BitVector type the same way we have the Vector type in gen_stubs.py.

@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch 2 times, most recently from c9d4f02 to 83ff6d2 Compare April 30, 2025 23:06
@cali-jumptrading cali-jumptrading changed the title WIP: flamenco: Add BitVector type flamenco: Add BitVector type May 1, 2025
@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch 3 times, most recently from 510f4dc to e0365d4 Compare May 1, 2025 15:40
@nlgripto nlgripto requested a review from anarcheuz May 1, 2025 18:01
ravyu-jump
ravyu-jump reviewed May 1, 2025
View reviewed changes
src/flamenco/types/gen_stubs.py
print(' ulong len;', file=body)
print(' err = fd_bincode_uint64_decode( &len, ctx );', file=body)
print(' if( FD_UNLIKELY( err!=FD_BINCODE_SUCCESS ) ) return err;', file=body)
print(f' if( len > inner_len * sizeof({self.vector_element}) * 8UL ) return FD_BINCODE_ERR_ENCODING;', file=body)
Copy link
Contributor

Choose a reason for hiding this comment

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

is the len < inner_len case impossible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

len can be less than inner_len * sizeof(element) * 8, we actually have a testcase called gossip_pull_req.yml that has this case. I think len might refer to the valid bits len?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i see

src/flamenco/runtime/sysvar/fd_sysvar_slot_history.c Outdated
inner->blocks_len = blocks_len;
inner->blocks_offset = (ulong)((uchar*)blocks - (uchar*)inner);
history->next_slot = slot_ctx->slot_bank.slot + 1UL;
history->bits_bitvec_offset = (ulong)((uchar*)history - (uchar*)history);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be

Suggested change
history->bits_bitvec_offset = (ulong)((uchar*)history - (uchar*)history);
history->bits_bitvec_offset = (ulong)((uchar*)blocks - (uchar*)history);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, nice catch!

@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch from e0365d4 to 5290dcf Compare May 1, 2025 22:54
@jumpsiegel
Copy link
Contributor

I swear I implemented bitVectors before but obviously that was in my previous job of implementing a solana validator... :)

jumpsiegel
jumpsiegel previously approved these changes May 2, 2025
View reviewed changes
@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch from 5290dcf to 0a0449d Compare May 2, 2025 14:25
@jumpsiegel jumpsiegel self-requested a review May 2, 2025 15:05
jumpsiegel
jumpsiegel previously approved these changes May 2, 2025
View reviewed changes
@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch from 0a0449d to 76368f3 Compare May 2, 2025 15:07
@jumpsiegel jumpsiegel requested a review from ravyu-jump May 2, 2025 15:54
jumpsiegel
jumpsiegel previously approved these changes May 2, 2025
View reviewed changes
ravyu-jump
ravyu-jump previously approved these changes May 2, 2025
View reviewed changes
Copy link
Contributor

@ravyu-jump ravyu-jump left a comment

Choose a reason for hiding this comment

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

Gossip side looks good

@cali-jumptrading cali-jumptrading dismissed stale reviews from ravyu-jump and jumpsiegel via baa8c00 May 2, 2025 19:40
@cali-jumptrading cali-jumptrading force-pushed the cali/fix-bitvector-type-short-term branch from 76368f3 to baa8c00 Compare May 2, 2025 19:40
@cali-jumptrading cali-jumptrading enabled auto-merge May 2, 2025 19:44
ibhatt-jumptrading
ibhatt-jumptrading approved these changes May 2, 2025
View reviewed changes
src/flamenco/types/gen_stubs.py
print(' err = fd_bincode_bool_decode( &o, ctx );', file=body)
print(' if( FD_UNLIKELY( err!=FD_BINCODE_SUCCESS ) ) return err;', file=body)
print(' if( o ) {', file=body)
self.vector_member.emitDecodeFootprint(' ')
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using numerical value to do indents instead of manual spaces

src/flamenco/types/gen_stubs.py

print(f' fun( w, NULL, "{self.name}", FD_FLAMENCO_TYPE_ARR_END, "array", level-- );', file=body)
# A BitVector can be modeled as an Option<Vector<some type>>
# See https://github.com/tov/bv-rs/blob/master/src/bit_vec/inner.rs#L8
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: tagged permalink

@cali-jumptrading cali-jumptrading added this pull request to the merge queue May 2, 2025
Merged via the queue into main with commit 5272011 May 2, 2025
9 checks passed
@cali-jumptrading cali-jumptrading deleted the cali/fix-bitvector-type-short-term branch May 2, 2025 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibhatt-jumptrading ibhatt-jumptrading ibhatt-jumptrading approved these changes

@jumpsiegel jumpsiegel jumpsiegel left review comments

@ravyu-jump ravyu-jump ravyu-jump left review comments

@anarcheuz anarcheuz Awaiting requested review from anarcheuz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cali-jumptrading @ripatel-fd @jumpsiegel @ravyu-jump @ibhatt-jumptrading