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

Integrate bulletproof rewinding #25

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

philipr-za
Copy link
Contributor

This PR integrates the rewinding of the dalek bulletproofs, currently implemented in the Tari fork of the bulletproofs crate, into tari_crypto. A method is provided that construct the rewindable range proof and two methods are provided to rewind the bulletproof to two levels, the first reveals the committed value and the proof message and the second will fully rewinds the bullet proof to also reveal the blinding factor.

While the underlying bulletproof implementation can accept a 23 byte proof message we use the first 4 bytes for a canonical message that lets us determine if the rewinding was successful or produced a garbage output. The remaining 19 bytes are provided for the client to use as they will

@hansieodendaal
Copy link
Contributor

Getting this error on Windows:

2020/11/30
09:35	Cargo project update failed:
			Execution failed (exit code 1).
			        C:/Users/pluto/.cargo/bin/cargo.exe metadata --verbose --format-version 1 --all-features
			        stdout : error: the 'cargo.exe' binary, normally provided by the 'cargo' component, is not applicable to the 'nightly-2020-09-15-x86_64-pc-windows-msvc' toolchain
			error: backtrace:
			error:    0: <unknown>
			   1: <unknown>
			   2: <unknown>
			   3: <unknown>
			   4: <unknown>
			   5: <unknown>
			   6: <unknown>
			   7: BaseThreadInitThunk
			   8: RtlUserThreadStart
			
			
			        stderr : 

09:35	Auto fetch: finished

@philipr-za
Copy link
Contributor Author

Getting this error on Windows:

2020/11/30
09:35	Cargo project update failed:
			Execution failed (exit code 1).
			        C:/Users/pluto/.cargo/bin/cargo.exe metadata --verbose --format-version 1 --all-features
			        stdout : error: the 'cargo.exe' binary, normally provided by the 'cargo' component, is not applicable to the 'nightly-2020-09-15-x86_64-pc-windows-msvc' toolchain
			error: backtrace:
			error:    0: <unknown>
			   1: <unknown>
			   2: <unknown>
			   3: <unknown>
			   4: <unknown>
			   5: <unknown>
			   6: <unknown>
			   7: BaseThreadInitThunk
			   8: RtlUserThreadStart
			
			
			        stderr : 

09:35	Auto fetch: finished

that doesn't look like an issue related to the PR?

Copy link
Contributor

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

Look good. A few maintainability and readability nits.

src/ristretto/dalek_range_proof.rs Outdated Show resolved Hide resolved
fn construct_proof_with_rewind_key(&self, key: &RistrettoSecretKey, value: u64, rewind_key: &RistrettoSecretKey, rewind_blinding_key: &RistrettoSecretKey, proof_message: &[u8; 19]) -> Result<Vec<u8>, RangeProofError> {
let mut pt = Transcript::new(b"tari");
let mut full_proof_message = [0u8; 23];
full_proof_message[0..4].clone_from_slice(b"tari");
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract message as a const; And I'd highly recommend making it 2 bytes to give a precious extra 2 bytes to the message. 1 byte is a bit short 1:256 chance of getting a false positive, 2 bytes is 1:65k.

Copy link
Contributor Author

@philipr-za philipr-za Nov 30, 2020

Choose a reason for hiding this comment

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

Will do, was just following the precedent set by the transcript but I agree.

The selection of how many bytes to use felt tricky. Happy to make it 2 for a 1 out of 65k chance of a false positive. Is that truly big enough? Maybe split the difference for a 3 byte check?

src/ristretto/dalek_range_proof.rs Outdated Show resolved Hide resolved
src/ristretto/dalek_range_proof.rs Outdated Show resolved Hide resolved
src/range_proof.rs Outdated Show resolved Hide resolved
src/range_proof.rs Outdated Show resolved Hide resolved
src/ristretto/dalek_range_proof.rs Outdated Show resolved Hide resolved
src/ristretto/dalek_range_proof.rs Outdated Show resolved Hide resolved
@hansieodendaal
Copy link
Contributor

Getting this error on Windows:

that doesn't look like an issue related to the PR?

It works perfectly with nightly-2020-06-10, but not with nightly-2020-09-15

@philipr-za
Copy link
Contributor Author

Getting this error on Windows:

that doesn't look like an issue related to the PR?

It works perfectly with nightly-2020-06-10, but not with nightly-2020-09-15

@CjS77 Shall we update the version in the repo?

@hansieodendaal
Copy link
Contributor

Could we perhaps run cargo fmt --all on this crate? I think it would improve readability.

@philipr-za
Copy link
Contributor Author

philipr-za commented Nov 30, 2020

Could we perhaps run cargo fmt --all on this crate? I think it would improve readability.

I was keen to but lets do it as a separate PR else it will confuse what this PR was updating.

@CjS77
Copy link
Contributor

CjS77 commented Nov 30, 2020

It works perfectly with nightly-2020-06-10, but not with nightly-2020-09-15

@CjS77 Shall we update the version in the repo?

Check the commit logs. IIRC there was a reason to move away from 2020-06-10. I forget the reason, but was Simd / WASM incompatibility or something.

Yup, see #23

CjS77
CjS77 previously approved these changes Nov 30, 2020
Copy link
Contributor

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

LGTM

@philipr-za
Copy link
Contributor Author

Had to update the branch of our fork of the Bulletproofs crate which was moved from main to development to reflect the branch naming of Daleks crate.

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

One comment below, and I think we should find a toolchain that also compiles on Windows.

src/ristretto/dalek_range_proof.rs Outdated Show resolved Hide resolved
@philipr-za
Copy link
Contributor Author

One comment below, and I think we should find a toolchain that also compiles on Windows.

Can you suggest a toolchain that works for windows and allows for the WASM issue?

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Please see suggested code change below.
Toolchain nightly-2020-09-16 looks promising for Windows builds; maybe for wasm_builder as well. (Note: nightly-2020-09-15 is broken for Windows.)

Cargo.toml Outdated Show resolved Hide resolved
@philipr-za
Copy link
Contributor Author

philipr-za commented Nov 30, 2020

@CjS77 I can't actually successfully run wasm-pack build . -- --features "wasm" for nightly-2020-09-15 or nightly-2020-09-16 but I can run it for nightly-2020-06-10

@philipr-za
Copy link
Contributor Author

Ok we tested nightly-2020-11-24 on ubuntu, mac os x and windows and it seems to work for the wasm build.

@CjS77
Copy link
Contributor

CjS77 commented Nov 30, 2020

@philipr-za Were you getting this issue on the 2020-09-15 build? rust-lang/rust#76698

Not sure why it worked when I was troubleshooting this before -- maybe the LLVM dylib was cached on the system or something.

@CjS77
Copy link
Contributor

CjS77 commented Nov 30, 2020

Confirmed: nightly-2020-11-24does not compile with-all-features`

nightly-2020-09-11 seems to work for wasm and all-features on Mac.

@philipr-za
Copy link
Contributor Author

@philipr-za Were you getting this issue on the 2020-09-15 build? rust-lang/rust#76698

Not sure why it worked when I was troubleshooting this before -- maybe the LLVM dylib was cached on the system or something.

Yes that was the exact issue I was having.

@hansieodendaal
Copy link
Contributor

hansieodendaal commented Nov 30, 2020

Toolchain nightly-2020-09-11 can be compiled on WIndows and Ubuntu

  • cargo build --all-features --release
  • wasm-pack build --target nodejs -d tari_js . -- --features "wasm"

However, the demo only works on Windows:

  • node wasm-demo

All toolchains tested with Ubuntu 18.04 LTS and Node.js 8.10.0 , even the prior nightly-2020-09-15, gives this error:

/home/pluto/@test/tari-crypto/tari_js/tari_crypto.js:80
const uint64CvtShim = new BigUint64Array(u32CvtShim.buffer);
                      ^

ReferenceError: BigUint64Array is not defined
    at Object.<anonymous> (/home/pluto/@test/tari-crypto/tari_js/tari_crypto.js:80:23)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/home/pluto/@test/tari-crypto/wasm-demo.js:25:19)
    at Module._compile (module.js:652:30)
~

Update: Working with the latest version of Node.js (nvm install v15)

@hansieodendaal
Copy link
Contributor

Update: Node.js requires v10+

This could probably be added to README.md

This PR integrates the rewinding of the dalek bulletproofs into tari_crypto. A method is provided that construct the rewindable range proof and two methods are provided to rewind the bullet proof to two levels, the first reveals the committed value and the proof message and the second will fully rewind the bullet proof to also reveal the blinding factor. 

While the underlying bulletproof implementation can accept a 23 byte proof message we use the first 4 bytes for a canonical message that lets us determine if the rewinding was successful or produced a garbage output. The remaining 19 bytes are provided for the client to use as they will
@CjS77 CjS77 merged commit d09e1ec into tari-project:main Dec 1, 2020
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.

3 participants