-
Notifications
You must be signed in to change notification settings - Fork 31
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
bitcoin: bump 0.28.0 -> 0.29.1 #147
Conversation
@@ -733,7 +733,7 @@ mod tests { | |||
// Initially secp context and rng global state | |||
let secp = secp256k1_zkp::Secp256k1::new(); | |||
#[allow(deprecated)] | |||
let mut rng = rand::ChaChaRng::seed_from_u64(0); | |||
let mut rng = rand::rngs::StdRng::seed_from_u64(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to docs StdRng use ChaCha, I think it's equivalent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it's deterministic anything should work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've errors in examples in comparison against test vectors ( pset_blind_coinjoin.rs
and raw_blind.rs
) which I think it's because those are not equivalent...
left: `Global { tx_data: TxData { version: 2, fallback_locktime: None, input_count: 2, output_count: 5, tx_modifiable: None }, version: 2, xpub: {}, scalars: [Tweak(a9fe9d44d70fd0efea5c2313c7235b02475b81bd243d2004b853821cee0b2047)], elements_tx_modifiable_flag: None, proprietary: {}, unknown: {} }`,
right: `Global { tx_data: TxData { version: 2, fallback_locktime: None, input_count: 2, output_count: 5, tx_modifiable: None }, version: 2, xpub: {}, scalars: [Tweak(0f56f3a1f798b585d605591085469b0e3d0c3f46c3bdf230773056302acccdb8)], elements_tx_modifiable_flag: None, proprietary: {}, unknown: {} }`', examples/pset_blind_coinjoin.rs:274:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
I can't keep rand 0.6 because it would break against secp256k1_zkp which is now 0.8
Any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can look into finding an equivalent here. 0.8 rand should have some equivalent PRNG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a deprecation notice on rand 0.6 referring to using the rand_chacha
crate (luckily wee need only as dev-dep), I tested it and it looks compatible:
let mut rng = rand_chacha::ChaChaRng::seed_from_u64(0);
assert_eq!(rng.next_u32(), 2180380594);
let mut rng = rand::ChaChaRng::seed_from_u64(0);
assert_eq!(rng.gen::<u32>(), 2180380594);
let mut rng = rand::rngs::StdRng::seed_from_u64(0);
assert_eq!(rng.gen::<u32>(),3384286946);
So I added c187bbb
and raw_blind.rs
example works, however, pset_blind_coinjoin.rs
still doesn't!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tempted to add a commit which removes the rand_chacha
dep, sticks in some custom crappy RNG, and updates the test vectors. We'd still have a commit that used rand_chacha
so that readers could check that the change in tests was only due to the change in RNG, rather than anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that should just be a separate PR..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pset_blind_coinjoin.rs still doesn't!
FYI this was due to wrongdoing in the "fix warning" commit
bb3e55b
to
cfe8e51
Compare
I've got a test failure which I am not sure how to tackle (to check the CI the test is temporarily ignored):
|
948416b Use crappy rng for raw_blind, update test_vector (Riccardo Casatta) ecb2c30 Use crappy rng for pset_blind_coinjoin, update test_vector (Riccardo Casatta) ed79cdd Move big test strings to external file (Riccardo Casatta) Pull request description: As suggested in #147 (comment) ACKs for top commit: apoelstra: ACK 948416b Tree-SHA512: 4eecb52ca357bc531b1872ed6e2ebc764627e226d0d56ed3a76b58590b27ce19aab3d8d99cd15978e4503bae2b60ad840a74f2e8a7a0f6caa85e94dde274658c
56f4512
to
ac20e2f
Compare
ac20e2f
to
8c976d1
Compare
Rebased over master, |
bitcoin_hases: bump 0.10.1 -> 0.11.0 rand: bump 0.6.5 -> 0.8 Code changes relate to: * Drop default from structure since hashes has all_zeros() instead of default(), Input manually implements default to minimize changes. * convert arrays to Scalar type
The serde error is due to So I think it's expected and I have update the test vector and mentioned in the CHANGELOG |
8c976d1
to
7a53252
Compare
Can you add a commit which renames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7a53252
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6039cc4
Do not merge yet, I've got issues in downstream lib on serde that wasn't there before the rename (suggestion appreciated, not sure what's happening)
|
Is there a |
Yes, but it's there like it was before the rename. |
Maybe your downstream crate is using a different version of secp-zkp now, and BTW we missed an opportunity to rename |
yeah sorry, there is no issue I was just missing a
yes, we should have done that, opening an issue -> BlockstreamResearch/rust-secp256k1-zkp#61 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6039cc4
bitcoin_hases: bump 0.10.1 -> 0.11.0
rand: bump 0.6.5 -> 0.8
Code changes relate to: