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.

Sign up for GitHub

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

Jump to bottom

feat: Use pregenerated ids for testing #284

Merged
merged 8 commits into from
Sep 17, 2018
Merged

feat: Use pregenerated ids for testing #284

merged 8 commits into from
Sep 17, 2018

Conversation

mkg20001
Copy link
Contributor

@mkg20001 mkg20001 commented Jul 31, 2018 •
edited by alanshaw
Loading

depends on ipfs/js-ipfs#1485

resolves #280

@mkg20001 mkg20001 mentioned this pull request Aug 4, 2018
Merged
@mkg20001 mkg20001 changed the title [WIP] feat: Use pregenerated ids for testing feat: Use pregenerated ids for testing Aug 4, 2018
alanshaw
alanshaw requested changes Aug 6, 2018
View reviewed changes
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Stunning work! 🚀

src/test-ids/index.js Outdated

module.exports.privKey = () => pbm.PublicKey.encode({
Type: pbm.KeyType.RSA,
Data: base.decode(Ids[rand(0, Ids.length)])
Copy link
Member

Choose a reason for hiding this comment

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

If we shuffle the array and then step through we'll not get any collisions until we use all of them up.

src/test-ids/index.js Outdated
module.exports.privKey = () => pbm.PublicKey.encode({
Type: pbm.KeyType.RSA,
Data: base.decode(Ids[rand(0, Ids.length)])
}).toString('base64')
Copy link
Member

Choose a reason for hiding this comment

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

Can we do all this work beforehand and have ids.json just be a list of base64 encoded private keys?

Copy link
Member

Choose a reason for hiding this comment

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

We'd also not need "whatever part of ascii can be put into a JSON string without needing to get escaped"-base if we did this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's bigger than. The comment starts with "compression" for a reason ;)
(I wanted to just use a binary protobuf but there are browsers and we know they like to mess with binary stuff)

src/test-ids/index.js Outdated
cb(null, new Id(digest, key, key.public))
})
})
}
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in ipfs/js-ipfs#1485 shall we just have a private key option? Do we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The reason why this even exists is because I added it for the in-process daemon first, then added the private key as string for external daemons.

@alanshaw
Copy link
Member

ping @mkg20001 :D

@mkg20001 mkg20001 force-pushed the feat/pregen-ids branch 2 times, most recently from bbc36ed to edaf163 Compare August 15, 2018 23:05
@mkg20001
Copy link
Contributor Author

@alanshaw Added requested changes

alanshaw
alanshaw requested changes Aug 16, 2018
View reviewed changes
src/ipfsd-daemon.js Outdated
@@ -78,7 +79,7 @@ class Daemon {
this._started = false
this.api = null
this.bits = this.opts.initOptions ? this.opts.initOptions.bits : null
this._env = Object.assign({}, process.env, this.opts.env)
this._env = Object.assign({}, process.env, this.opts.env, this.disposable ? {IPFS_PREGENERATED_PRIVATE_KEY: testIds()} : {})
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be passed as a command line option instead now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, missed that one

src/test-ids/index.js Outdated
return pbm.PublicKey.encode({ // this is needed because the keys aren't wrapped in the usual privateKey pbuf container to save space
Type: pbm.KeyType.RSA,
Data: base.decode(Ids.pop())
}).toString('base64')
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, how much bigger would the file be? It's currently ~250kb. I'm guessing you've checked this encode/decode/toString has negligible overhead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

~444kb would be the filesize for a base64 array

@daviddias daviddias added the status/ready Ready to be worked label Aug 25, 2018
@hugomrdias hugomrdias mentioned this pull request Aug 27, 2018
Closed
@daviddias
Copy link
Member

What's the ETA to ship this?

@mkg20001
Copy link
Contributor Author

I missed the new review. Added the changes.

@alanshaw
Copy link
Member

What's the ETA to ship this?

I've been blocking this with ipfs/js-ipfs#1485 - sorry, more important things delayed me! Hoping to unblock this week.

@alanshaw
chore: upgrade to latest ipfs version
e9cc83c 
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@ghost ghost assigned alanshaw Sep 12, 2018
@ghost ghost added status/in-progress In progress and removed status/ready Ready to be worked labels Sep 12, 2018
@daviddias
Copy link
Member

@hugomrdias can you do the final review and handle the merge and release?

@hugomrdias
Copy link
Member

there's still stuff pending from my last review

@mkg20001
Copy link
Contributor Author

@hugomrdias I couldn't find anything. Mind linking it?

hugomrdias
hugomrdias reviewed Sep 17, 2018
View reviewed changes
src/test-ids/index.js Outdated
const pbm = protobuf(require('libp2p-crypto/src/keys/keys.proto'))

module.exports = () => {
if (!Ids.length) { // TODO: should this maybe re-use ids?
Copy link
Member

Choose a reason for hiding this comment

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

can we add a note in the readme for disposable daemons explaining this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or should this maybe re-use ids ??

Copy link
Member

Choose a reason for hiding this comment

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

Print a warning and just stop using pregenerated ones?

src/test-ids/index.js Outdated
return a
}

const Ids = shuffle(require('./ids.json').split('"')) // file can be generated using mkg20001/test-peer-ids.tk patched with ipfs://QmVZRWjZoqve9UKgng2gymN8a3FQUENgsQaicQ2fWs7kGx
Copy link
Member

Choose a reason for hiding this comment

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

pls change Ids to ids

src/test-ids/index.js Outdated
const Ids = shuffle(require('./ids.json').split('"')) // file can be generated using mkg20001/test-peer-ids.tk patched with ipfs://QmVZRWjZoqve9UKgng2gymN8a3FQUENgsQaicQ2fWs7kGx
const base = require('base-x')(' !#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_`abcdefghijklmnopqrstuvwxyz{|}~') // "compression" aka "whatever part of ascii can be put into a JSON string without needing to get escaped"-base

const protobuf = require('protons')
Copy link
Member

Choose a reason for hiding this comment

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

pls move all the requires to the top, just a nitpick for consistency :)

@hugomrdias
Copy link
Member

ups the pending part was on my side lol, forgot to click to send review.
sorry should be visible now

@daviddias daviddias merged commit cbc1ac6 into ipfs:master Sep 17, 2018
@ghost ghost removed the status/in-progress In progress label Sep 17, 2018
hugomrdias added a commit that referenced this pull request Oct 10, 2018
@hugomrdias
Revert "feat: Use pregenerated ids for testing (#284)"
1d38cd2 
This reverts commit cbc1ac6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugomrdias hugomrdias hugomrdias left review comments

@alanshaw alanshaw alanshaw approved these changes

Assignees

@hugomrdias hugomrdias

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

speed boost: gen 200 PeerIds in advance
4 participants
@mkg20001 @alanshaw @daviddias @hugomrdias