Random Number Generation #2053

lrubiorod · 2021-09-02T15:58:09Z

Close #2056

data_structures/src/data_request.rs

rad/src/reducers/hash_concatenate.rs

tmpolaczyk · 2021-09-07T10:24:55Z

validations/src/validations.rs

+    println!(
+        "expected_ta_tx.tally: {}",
+        hex::encode(&expected_ta_tx.tally)
+    );


This should be removed

I added it to help me write the tests, if the tests are done we don't need it.

I had to modify several times the rng tests, so I think it could be useful in the future too

data_structures/src/data_request.rs

aesedepece · 2021-09-07T15:59:52Z

rad/src/lib.rs

+    if is_rng {
+        Ok(RadonReport::from_result(
+            Ok(radon_types_vec[0].clone()),
+            context,
+        ))
+    } else {


What is this doing?

unwrap() :P

In the rng case the request will usually have only one retrieval source. So this way we avoid the aggregation step, which is not needed. However I think that could apply to all kinds of requests, because you don't need aggregation if there is only one source. This would be a breaking change, but we can implement it with TAPI.

I removed it and use the Mode instead, because the mode of a vector of one element is that element. To avoid strange cases

rad/src/reducers/hash_concatenate.rs

aesedepece · 2021-09-07T16:44:50Z

validations/src/validations.rs

+            if path.url.is_empty() || path.script.is_empty() {
+                is_rng = true;
+                break;
+            } else {
+                return Err(DataRequestError::InvalidRngRequest.into());
+            }


I don't really follow the logic here hehehe.

I can see why url must be empty (won't be used).

I have doubts on whether script should be allowed or not.

By reading this code, I understand that as long as url is empty, script can be non-empty, but will it be used anyway?

Some general but related comments:

RADTypes only exist at the data source / retrieval level. We should avoid handling a data request differently only because one of the data sources is of a special kind.

There won't exist such thing as RNG requests, only requests with RNG sources.

RNG and HttpGet sources should coexist in the same data request as long as they have the same output type.

E.g. in the future we may have a data source type that performs general computation by running WASM bytecode in a sandbox.

And you may want to use that side by side with a classic HttpGet data source that triggers a similar computation on some cloud, or fetches similar outputs from some API.

My point is that you should perfectly be able to do so, and this part of the code should only care about routing the data source definitions to their "resolvers" (the resolver of HttpGet is our wrapper of the Surf library, the resolver of Rng is the RNG and the resolver of the hypothetical Wasm type would be a WASM virtual machine).

Agree that is_rng is a hack and we should find a way to avoid it.

aesedepece · 2021-09-07T16:52:17Z

validations/src/validations.rs

        if path.url.is_empty() {
            return Err(DataRequestError::NoRetrievalSources.into());
        }


Won't this cause an early return for any retrieval path / data source of RADType::Rng that has both url and script being empty?

In the rng case it does never reach this line because there is a break in the previous condition. But that limits rng requests to one retrieval source, so we need to change this logic.

If it is a RADType::Rng, it will always return earlier in the previous branchs (break or DataRequestError::InvalidRngRequest)

aesedepece · 2021-09-07T16:53:54Z

validations/src/validations.rs

+    if is_rng && retrieval_paths.len() > 1 {
+        return Err(DataRequestError::InvalidRngRequest.into());
+    }


As per my general comment above, this goes against the three points.

node/tests/data_request_examples.rs

tmpolaczyk · 2021-09-09T09:34:47Z

rad/src/script.rs

+        | RadonReducers::Mode
+        | RadonReducers::AverageMedian
+        | RadonReducers::HashConcatenate
+        | RadonReducers::Unwrap => {}


This function needs a context, the new operators can only be considered valid if the WIP is active.

It is fixed in #2063, because it also affect to the median

aesedepece · 2021-09-10T08:01:35Z

rad/src/reducers/mod.rs

 #[repr(u8)]
 pub enum RadonReducers {
    // Implemented
    Mode = 0x02,
    AverageMean = 0x03,
    AverageMedian = 0x05,
    DeviationStandard = 0x07,
+    HashConcatenate = 0x0b,
+    Unwrap = 0x0c,


Interesting. Where did this come from?

We dont have a reducer to convert an array of 1 element in that element

Deleted unwrap, using Mode instead of

tmpolaczyk · 2021-09-10T10:29:50Z

rad/src/lib.rs

+            let random_bytes: [u8; 32] = rand::random();
+            let random_bytes = RadonTypes::from(RadonBytes::from(random_bytes.to_vec()));
+
+            Ok(RadonReport::from_result(Ok(random_bytes), context))


This ignores the retrieval script, according to the WIP the script should be executed.

Well actually I'm not sure if the retrieval script should be executed or not.

Let's discuss here:
witnet/WIPs#70 (comment)

What was the conclusion?

tmpolaczyk · 2021-09-13T10:56:15Z

Using commit 0c4ffec and this data request:

{"jsonrpc":"2.0","method":"sendRequest","id":"1","params":{"dro":{"data_request":{"time_lock":0,"retrieve":[{"kind":"RNG","url":"","script":[128]}],"aggregate":{"filters":[],"reducer":3},"tally":{"filters":[],"reducer":3}},"witness_reward":1000,"witnesses":5,"commit_and_reveal_fee":10,"min_consensus_percentage":51,"collateral":1000000000},"fee":0}}

An updated node accepts that as a valid request, while a not-updated node does not, leading to a fork.

mariocao · 2021-09-17T10:34:25Z

rad/src/lib.rs

+/// Auxiliary function that returns the current active wips and the WIPs in voting process as actived
+/// It is only used for testing
+pub fn all_wips_active() -> ActiveWips {
+    let mut h = current_active_wips();


Why h? 🤣

It stands for HashMap, because the first version of ActiveWips was just a HashMap.

rad/src/lib.rs

…e instead of

…tivation Co-authored-by: Tomasz Polaczyk <tmpolaczyk@gmail.com>

…idations

holidays!

lrubiorod force-pushed the random_numbers branch 3 times, most recently from 86ffe89 to 15b7bc5 Compare September 6, 2021 13:53

tmpolaczyk reviewed Sep 6, 2021

View reviewed changes

data_structures/src/data_request.rs Outdated Show resolved Hide resolved

lrubiorod changed the base branch from master to witnet-1.4 September 7, 2021 09:42

tmpolaczyk reviewed Sep 7, 2021

View reviewed changes

rad/src/reducers/hash_concatenate.rs Outdated Show resolved Hide resolved

tmpolaczyk reviewed Sep 7, 2021

View reviewed changes

rad/src/reducers/hash_concatenate.rs Outdated Show resolved Hide resolved

tmpolaczyk reviewed Sep 7, 2021

View reviewed changes

data_structures/src/data_request.rs Show resolved Hide resolved

lrubiorod force-pushed the random_numbers branch from 2db052b to d5bbf32 Compare September 7, 2021 11:45

lrubiorod marked this pull request as ready for review September 7, 2021 11:46

lrubiorod force-pushed the random_numbers branch 2 times, most recently from 4659b63 to 58e5d0b Compare September 7, 2021 15:57

aesedepece reviewed Sep 7, 2021

View reviewed changes

rad/src/reducers/hash_concatenate.rs Show resolved Hide resolved

aesedepece reviewed Sep 7, 2021

View reviewed changes

lrubiorod force-pushed the random_numbers branch from a928182 to a334c8c Compare September 8, 2021 13:32

lrubiorod changed the title ~~(wip) Random Number Generation~~ Random Number Generation Sep 8, 2021

tmpolaczyk reviewed Sep 8, 2021

View reviewed changes

node/tests/data_request_examples.rs Outdated Show resolved Hide resolved

tmpolaczyk reviewed Sep 9, 2021

View reviewed changes

lrubiorod force-pushed the random_numbers branch 2 times, most recently from e8b3537 to fe14ad9 Compare September 9, 2021 16:05

aesedepece previously requested changes Sep 10, 2021

View reviewed changes

tmpolaczyk reviewed Sep 10, 2021

View reviewed changes

lrubiorod force-pushed the witnet-1.4 branch from d248a53 to fb70fcc Compare September 10, 2021 13:15

lrubiorod force-pushed the random_numbers branch from fe14ad9 to 0c4ffec Compare September 10, 2021 14:00

tmpolaczyk mentioned this pull request Sep 13, 2021

RADType::Rng is decoded as RADType::HttpGet (protobuf defaults unknown enum values to 0) #2066

Closed

mariocao reviewed Sep 17, 2021

View reviewed changes

rad/src/lib.rs Show resolved Hide resolved

lrubiorod force-pushed the random_numbers branch from b54dd20 to dca1c0f Compare September 17, 2021 11:02

mariocao approved these changes Sep 17, 2021

View reviewed changes

lrubiorod force-pushed the random_numbers branch from dca1c0f to 2ca0fe9 Compare September 17, 2021 11:26

lrubiorod and others added 21 commits September 17, 2021 15:04

feat(rad): add RNG in retrieval RADType

556adfd

feat(rad): skipping aggregation when rng

d3a7af7

feat(rad): implement HashConcatenate reducer

bf45289

feat(rad): include tapi activation for includeing RNG functionality

61c052f

feat(data_request): modify sorting in get_all_reveals

e769da5

feat(rad): during RNG, ensure reveal bytes has the same lenght

5b92d2b

feat: add random_bytes.json data request example

ff41187

fix(tapi): include wip19 in tapi initialize test

5115fbc

test: tally rng request

5e0cdd9

refactor: improve and test hash concatenate

799fb3f

refactor(rad): modify HashConcatenate reducer code

9fd5df6

feat(rad): add unwrap reducer

f317060

fix(rad): fix rng tally tests

d6639ac

feat(validation): remove extra validations for rng

044c670

feat(rad): include tapi in RadType validation

619ae2a

refactor(rad): remove option for active_wips

beeac1f

fix(tapi): fix tests after include RadType changes

a29a77a

refactor(rad): remove RadonReducers::Unwrap, using RadonReducers::Mod…

a744726

…e instead of

feat(rad): convert RadType from HttpGet to Unknown before WIP-0019 ac…

f8e1540

…tivation Co-authored-by: Tomasz Polaczyk <tmpolaczyk@gmail.com>

refactor: move current_active_wips and all_wips_active to mainnet_val…

5e35b2c

…idations

feat: apply suggestions

1b3176c

lrubiorod force-pushed the random_numbers branch from 2ca0fe9 to 1b3176c Compare September 17, 2021 13:05

tmpolaczyk approved these changes Sep 17, 2021

View reviewed changes

lrubiorod merged commit 1b3176c into witnet:master Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Random Number Generation #2053

Random Number Generation #2053

lrubiorod commented Sep 2, 2021 •

edited

Loading

tmpolaczyk Sep 7, 2021

lrubiorod Sep 7, 2021

tmpolaczyk Sep 7, 2021

lrubiorod Sep 15, 2021

aesedepece Sep 7, 2021

lrubiorod Sep 7, 2021

tmpolaczyk Sep 8, 2021

lrubiorod Sep 15, 2021

aesedepece Sep 7, 2021

aesedepece Sep 7, 2021 •

edited

Loading

aesedepece Sep 7, 2021

tmpolaczyk Sep 8, 2021

aesedepece Sep 7, 2021

tmpolaczyk Sep 8, 2021

lrubiorod Sep 8, 2021

aesedepece Sep 7, 2021

tmpolaczyk Sep 9, 2021

lrubiorod Sep 9, 2021

aesedepece Sep 10, 2021

lrubiorod Sep 10, 2021

lrubiorod Sep 15, 2021

tmpolaczyk Sep 10, 2021

tmpolaczyk Sep 10, 2021

aesedepece Sep 10, 2021

lrubiorod Sep 15, 2021

tmpolaczyk commented Sep 13, 2021

mariocao Sep 17, 2021

tmpolaczyk Sep 17, 2021

Random Number Generation #2053

Random Number Generation #2053

Conversation

lrubiorod commented Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aesedepece Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmpolaczyk commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrubiorod commented Sep 2, 2021 •

edited

Loading

aesedepece Sep 7, 2021 •

edited

Loading