js-libp2p
js-libp2p copied to clipboard
libp2p
do not trigger attack protection when loading self
If the repo is empty, then calling await this.keychain.findKeyByName('self') will trigger await randomDelay() which attempts to slow down some kinds of attacks. We are not attacking ourselves, and so we should check for the self key gracefully.
Further, there appears no purpose for the try/catch block since its original use was to catch the error thrown by findKeyByName('self')
This is a good spot and will speed up initial startup, thanks for submitting it.
I think it would be better to refactor the KeyChain class to make it implement the Startable interface and add the 'self' key to the datastore in the start method (if not already in the datastore), and remove the loadKeychain method entirely.
See the PeerRecordUpdater class for an example of a Startable.
Ok thanks for the feedback. I cannot afford to make those changes in the short term as we need to launch our work management app, but I'll insert this task into said app with some funds against it and try get it picked picked up by someone with a free slot.
@mpetrunic pardon, but what is the further 'need/author-input' that is required here ?
Our presumption was that @achingbrain was simply laying out a better solution, while also indicating this patch was an improvement in the short term. I don't know how he manages to do as much coding as he does, but it is probably very difficult to spec out all the little changes he'd like but hasn't time to get to - this is one such change, and we'll loop back to it when we can with a new PR.
Hence I did not think any further input was required on this particular patch ?
@mpetrunic pardon, but what is the further 'need/author-input' that is required here ?
Our presumption was that @achingbrain was simply laying out a better solution, while also indicating this patch was an improvement in the short term. I don't know how he manages to do as much coding as he does, but it is probably very difficult to spec out all the little changes he'd like but hasn't time to get to - this is one such change, and we'll loop back to it when we can with a new PR.
Hence I did not think any further input was required on this particular patch ?
@inverted-capital We were under the impression, that you will update this PR once you find time for it so We assigned a new tag so it doesn't show during triage meetings. Should we close PR and open issue instead?
Hey @mpetrunic - thanks for the response - yes if you could merge this PR (provided it is ok!) and open an issue for the new work that needs to be done, that would be great. I can open the issue if you'd like ?
Hey @mpetrunic - thanks for the response - yes if you could merge this PR (provided it is ok!) and open an issue for the new work that needs to be done, that would be great. I can open the issue if you'd like ?
Hey @inverted-capital, we chatted internally and if this is not an absolute emergency, we would rather wait for a proper fix than merge this. If we merge this, we don't believe that it would get properly fixed.
