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

Make nodes commit encode error when aggregation script result cannot be encoded as CBOR #2312

Open
tmpolaczyk opened this issue Nov 21, 2022 · 2 comments
Labels
breaking change ⚠️ Introduces breaking changes

Comments

@tmpolaczyk
Copy link
Contributor

Issue #2310 describes a radon script whose result cannot be encoded. If that happens, nodes will not commit any value and the data request will probably resolve with "insufficient commits" error. We could have nodes report an "encode" error to detect this case. There is already a RadError::Encode error, we could reuse it or we could create a more specific EncodeAggregationResultToCbor error.

This is the code that decides to ignore this case:

Err(e) => {
log::error!("Couldn't decode tally value from bytes: {}", e);
actix::fut::err(())
}

I tried to find similar issues to #2310, which fails to encode integers greater than 2^64-1, but didn't find any yet. But anyway, it would be great to have that in case something unexpectedly breaks in the future.

@tmpolaczyk
Copy link
Contributor Author

So unfortunately it looks like RadError::Encode is not a valid RadonError, so we cannot use that. Not sure why, but it looks like if we want to fix this issue we will have to introduce new radon error codes.

See the test from this branch:

https://github.com/tmpolaczyk/witnet-rust/tree/encode-encode-error

it fails with an encode error

thread 'actors::chain_manager::mining::tests::encode_error_can_be_encoded' panicked at 'called `Result::unwrap()` on an `Err` value: Encode { from: "RadonTypes", to: "Vec<u8>" }', node/src/actors/chain_manager/mining.rs:1617:12

@tmpolaczyk tmpolaczyk added the breaking change ⚠️ Introduces breaking changes label Nov 24, 2022
@aesedepece
Copy link
Member

So unfortunately it looks like RadError::Encode is not a valid RadonError, so we cannot use that. Not sure why, but it looks like if we want to fix this issue we will have to introduce new radon error codes.

That one is definitely needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Introduces breaking changes
Projects
None yet
Development

No branches or pull requests

2 participants