Skip to content

Fix outpoint_state index store#194

Merged
dr-orlovsky merged 1 commit intoRGB-WG:masterfrom
zoedberg:fix/outpoint_state
Aug 22, 2022
Merged

Fix outpoint_state index store#194
dr-orlovsky merged 1 commit intoRGB-WG:masterfrom
zoedberg:fix/outpoint_state

Conversation

@zoedberg
Copy link
Member

@zoedberg zoedberg commented Aug 12, 2022

Found a bug calling outpoint_state. By calling the method with an empty Vec<OutPoint> it correctly returns known allocations, but by calling it with some specific outpoints it returns no allocations.

Investigating this I've found that when storing the index into the DB:

for seal in genesis.revealed_seals().unwrap_or_default() {
    debug!("Adding outpoint for seal {}", seal);
    let index_id = ChunkId::with_fixed_fragments(seal.txid, seal.vout);
    self.store.insert_into_set(db::OUTPOINTS, index_id, contract_id)?;
}

ChunkId::with_fixed_fragments receives a Option<Txid>.

While, when searching for specific outpoints:

let indexes = if outpoints.is_empty() {
    self.store.ids(db::OUTPOINTS)?
} else {
    outpoints
        .iter()
        .map(|outpoint| ChunkId::with_fixed_fragments(outpoint.txid, outpoint.vout))
        .collect()
};

ChunkId::with_fixed_fragments receives a Txid.

I've fixed the bug on the store-side by unwrapping the Option, since I believe the TXID should always be present at that point, I hope that's correct.

I manually tested it and can confirm that with this fix outpoint_state filter works as expected.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Thank you for catching this!

While your fix is correct, rust stongly discourage use of unwraps in any production code; the unwrap_or, unwrap_or_default or expect should be used instead - or return an error.

I think we should do a expect("genesis with vout-based seal which passed schema validation"), to indicate the reason of the crash (genesis must validate absece of vout-based seals at the schema level, and if it is present, the line 167 and 181 above should guarantee that we would never reach this place).

Can you pls update your PR to use expect?

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Aug 21, 2022
@dr-orlovsky dr-orlovsky added this to the 0.8.0 milestone Aug 21, 2022
@zoedberg zoedberg force-pushed the fix/outpoint_state branch from 6368491 to e737bd6 Compare August 22, 2022 08:44
@zoedberg
Copy link
Member Author

sure, thanks for the tip. fixed

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK e737bd6

@dr-orlovsky
Copy link
Member

I think this is a breaking change that will invalidate the stash for existing RGB nodes. Will reflect in a release notes of the patch version...

@dr-orlovsky dr-orlovsky merged commit 18289f8 into RGB-WG:master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

2 participants