Skip to content
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.

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

Add merging to BlockRange #3923

Merged
merged 3 commits into from
Apr 12, 2024
Merged

Add merging to BlockRange #3923

merged 3 commits into from
Apr 12, 2024

Conversation

nick-mobilecoin
Copy link
Collaborator

No description provided.

Copy link

⚠️ Downstream repo mobilecoinofficial/full-service failed to build. Check actions status for details.

Copy link

⚠️ Downstream repo mobilecoinofficial/android-bindings failed to build. Check actions status for details.

1 similar comment
Copy link

⚠️ Downstream repo mobilecoinofficial/android-bindings failed to build. Check actions status for details.

Copy link

⚠️ Downstream repo mobilecoinofficial/full-service failed to build. Check actions status for details.

@nick-mobilecoin
Copy link
Collaborator Author

from internal convo

I’m curious:

  1. how you imagine using index
  2. what utility there is in creating a merged_range with merged_range.start_block != 0
  1. The index was added to support determining how to populate the CheckedKeyImageResponse,

    let num_blocks = shard_query_responses
    . My hope is that with the index the caller will then be able to copy that response directly from the shard that provided the "final" range that got merged. I'm up for ideas on this one. I think we need to ensure that the caller doesn't accidentally provide a larger global txo count than the number of blocks. Lets assume there are 3 txos per block and the ranges are (0,10), (10, 12), (20, 22). The intent is that our max block returned is 12, I'm thinking we also need to ensure the gobal_txo_count is 36 and not 66.

  2. I don't think there is any use in providing a merged range that doesn't start at 0, for the system. However for this function I think it's already complicated enough with the index piece. My thought was that the caller would do something like (non compiling code)

let zero_range = BlockRange::new(0,1);
let received_ranges = ranges.sort(); // some logic somewhere that keeps the indices when sorted.
let (index, merged_range) = BlockRange.merge_from_ranges([&zero_range].chain(received_ranges));

Up for suggestions as I recognize this may be placing too much room for error by the callers

@holtzman
Copy link
Contributor

from internal convo

I’m curious:

  1. how you imagine using index
  2. what utility there is in creating a merged_range with merged_range.start_block != 0
  1. The index was added to support determining how to populate the CheckedKeyImageResponse,
    let num_blocks = shard_query_responses

    . My hope is that with the index the caller will then be able to copy that response directly from the shard that provided the "final" range that got merged. I'm up for ideas on this one. I think we need to ensure that the caller doesn't accidentally provide a larger global txo count than the number of blocks. Lets assume there are 3 txos per block and the ranges are (0,10), (10, 12), (20, 22). The intent is that our max block returned is 12, I'm thinking we also need to ensure the gobal_txo_count is 36 and not 66.
  2. I don't think there is any use in providing a merged range that doesn't start at 0, for the system. However for this function I think it's already complicated enough with the index piece. My thought was that the caller would do something like (non compiling code)
let zero_range = BlockRange::new(0,1);
let received_ranges = ranges.sort(); // some logic somewhere that keeps the indices when sorted.
let (index, merged_range) = BlockRange.merge_from_ranges([&zero_range].chain(received_ranges));

Up for suggestions as I recognize this may be placing too much room for error by the callers

Ah. Fessing up to initiating the internal conversation, and this makes sense to me.

fog/types/src/common.rs Outdated Show resolved Hide resolved
@holtzman
Copy link
Contributor

Up for suggestions as I recognize this may be placing too much room for error by the callers

What if each BlockRange included the query_response from the corresponding shard? Then instead of doing a merge_from_range, do abest_query_response where all of the shard query_responses are sorted by start_block and looked at in order until a discontinuity is found (or the end of the sorted BlockRange list is reached) and the query_response from before the discontinuity (or final one in list if there is no discontinuity) is returned?

Then num_blocks, global_txo_count, and latest_block_version could be taken from that query_response.

Some logic would also be needed in case none of the shards reported results that included block 0.

@nick-mobilecoin
Copy link
Collaborator Author

Up for suggestions as I recognize this may be placing too much room for error by the callers

What if each BlockRange included the query_response from the corresponding shard? Then instead of doing a merge_from_range, do abest_query_response where all of the shard query_responses are sorted by start_block and looked at in order until a discontinuity is found (or the end of the sorted BlockRange list is reached) and the query_response from before the discontinuity (or final one in list if there is no discontinuity) is returned?

Then num_blocks, global_txo_count, and latest_block_version could be taken from that query_response.

Some logic would also be needed in case none of the shards reported results that included block 0.

This may be a better alternative. The shards are going to need to return a different type then the CheckKeyImagesResponse in order to accomodate the range. So it may make sense to create a dedicated NewResponseType::merge_from(responses) and contain the logic there.

I'll keep this PR open/unmerged for now and build the other logic on top of this to see how it plays out.

@nick-mobilecoin
Copy link
Collaborator Author

Follow on PR is #3933

Co-authored-by: Henry Holtzman <henry@mobilecoin.com>
Copy link

⚠️ Downstream repo mobilecoinofficial/android-bindings failed to build. Check actions status for details.

Copy link

⚠️ Downstream repo mobilecoinofficial/full-service failed to build. Check actions status for details.

Copy link

⚠️ Downstream repo mobilecoinofficial/android-bindings failed to build. Check actions status for details.

Copy link

⚠️ Downstream repo mobilecoinofficial/full-service failed to build. Check actions status for details.

@nick-mobilecoin nick-mobilecoin merged commit a6cd3b5 into release/v6.0 Apr 12, 2024
100 checks passed
@nick-mobilecoin nick-mobilecoin deleted the nick/range-logic branch April 12, 2024 14:18
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.

None yet

2 participants