Integrated FMD indices into shielded sync logic

[batconjurer]

Describe your changes

Closes #4621

For the MASP Indexer client, now only fetches of MASP txs included in an index set computed via FMD if such a set is present. Also uses the set to reduce the number of trial decryptions. This PR does not handle loading in FMD index sets. That will be determined later.

Checklist before merging

• If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
  • Relevant PR if applies:

[murisi]

Took a look through, thanks. Just have some questions about the index arithmetic.

[murisi]

check_block_index seems to expect from and to to form an inclusive range. (Its use of the strict inequalities block_index_height > to and block_index_height < from seems to indicate this.) However from_height + offset, the supplied argument, seems to be an exclusive upper bound. Shouldn't block_index_height be considered to be above even when it equals from_height + offset?

[batconjurer]

from_height + offset should be an inclusive upper bound. What makes it seem exclusive? Maybe there is an issue elsewhere if you got that impression

[murisi]

Thanks for clarifying. My thought process is as follows: the from_height and offset values here essentially come from

namada/crates/sdk/src/masp/utilities.rs

Line 503 in 41c2e84

self.fetch_request(from_height, offset, maybe_block_index)

. So from_height + offset (inside fetch_request) is equal (?) to to_height at the point when fetch_request is called (in fetch_shielded_transfers). Also, it seems that the to_height of an iteration of

namada/crates/sdk/src/masp/utilities.rs

Line 494 in 41c2e84

while from <= to {

becomes the from_height of the next iteration because of

namada/crates/sdk/src/masp/utilities.rs

Line 500 in 41c2e84

from += offset;

. So for it to be true that this code does not double process boundary blocks (as the loop iterates), I assumed that from_height is inclusive and to_height is an exclusive bound. What am I missing?

[batconjurer]

To avoid the double processing, I added this line

namada/crates/sdk/src/masp/utilities.rs

Line 537 in 41c2e84

if from == to {

[batconjurer]

If you recall, this originally had the labelled outer loop which was called do_while. It's annoying we don't have a do while loop in rust since it's exactly what we need here.

[murisi]

To avoid the double processing, I added this line

namada/crates/sdk/src/masp/utilities.rs

Line 537 in 41c2e84

if from == to {

Thanks for clarifying @batconjurer . While I see how this logic would prevent double processing at the block index to, I'm not sure how this logic prevents double processing at to_height values before this. To try and understand the loop (both before this PR and also after this PR) better, I've tried to strip it down to just the index operations:

fn old_style() {
    println!("Old style at commit a4ebd7416d4d32d027f3efcde60ffd0a01ebd3cb");
    let mut from: u64 = 0;
    const to: u64 = 33;
    loop {
        'do_while: {
            const MAX_RANGE_THRES: u64 = 10;

            let mut from_height = from;
            let mut offset = (to - from).min(MAX_RANGE_THRES);
            let mut to_height = from + offset;
            from += offset;

            println!("check_block_index(_, {}, {}); // from_height = {}, to_height = {}", from_height, to_height, from_height, to_height);
        }

        if from >= to {
            break;
        }
    }
}

fn new_style() {
    println!("New style at commit 41c2e8406e62e93265d75d1eeda14881ffa402ee");
    let mut from: u64 = 0;
    const to: u64 = 33;
    while from <= to {
        const MAX_RANGE_THRES: u64 = 10;

        let from_height = from;
        let offset = (to - from).min(MAX_RANGE_THRES);
        let to_height = from + offset;
        from += offset;

        println!("fetch_request({}, {}, _); // from_height = {}, to_height = {}", from_height, offset, from_height, to_height);

        if from == to {
            break;
        }
    }
}

fn main() {
    old_style();
    new_style();
}

Is the above code representative of the index computations? If so, executing the above code yields the following outputs:

Old style at commit a4ebd7416d4d32d027f3efcde60ffd0a01ebd3cb
check_block_index(_, 0, 10); // from_height = 0, to_height = 10
check_block_index(_, 10, 20); // from_height = 10, to_height = 20
check_block_index(_, 20, 30); // from_height = 20, to_height = 30
check_block_index(_, 30, 33); // from_height = 30, to_height = 33
New style at commit 41c2e8406e62e93265d75d1eeda14881ffa402ee
fetch_request(0, 10, _); // from_height = 0, to_height = 10
fetch_request(10, 10, _); // from_height = 10, to_height = 20
fetch_request(20, 10, _); // from_height = 20, to_height = 30
fetch_request(30, 3, _); // from_height = 30, to_height = 33

If from_height and to_height are supposed to be interpreted as inclusive boundaries, then it seems to me that the indices 10, 20, and 30 would be double processed by check_block_index in the old style, and by fetch_request in the new style. Would it be possible to explain why this doesn't happen or, if it does happen, why it's not an issue? I really think I'm just missing something basic here... Thanks!

[batconjurer]

Oh, I see. Ok, I'll go in and try to fix this. I think it should work if we make the following two changes: from+=offset+1 and then I believe we can remove the from==to check.

[batconjurer]

Oh goddammit. It turns out we've been always double processing the endpoint due to an inconsistency on inclusive vs exclusive bounds. But it resolves itself later because of a btreemap used by the dispatcher that removes those duplicates.

[batconjurer]

Ok, it's been resolved.