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

[NayNay] Signing bug for validators #63

Merged
merged 5 commits into from
Jun 12, 2024
Merged

[NayNay] Signing bug for validators #63

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad43e99
[NayNay] Signing bug for validators
rh0delta Jun 3, 2024
b729559
reset attempts counter on successful signature
rh0delta Jun 4, 2024
88d0282
Merge branch 'main' into naynay/signing-validators-bug
rh0delta Jun 5, 2024
fa291d2
adding do not delete to comments
rh0delta Jun 5, 2024
71ee8b9
fixed the race condition as per mixs suggestions
rh0delta Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"dependencies": {
"@entropyxyz/sdk": "^0.1.5-5",
"@ethereumjs/util": "^9.0.2",
"@polkadot/util": "^12.6.2",
rh0delta marked this conversation as resolved.
Show resolved Hide resolved
"@types/node": "^20.12.12",
"alchemy-sdk": "^3.1.2",
"ansi-colors": "^4.1.3",
Expand Down
121 changes: 78 additions & 43 deletions src/flows/sign/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,84 @@
import inquirer from "inquirer"
import { u8aToHex } from '@polkadot/util'
import { initializeEntropy } from "../../common/initializeEntropy"
import { debug, getSelectedAccount, print } from "../../common/utils"

let msg: string
let signingAttempts: number = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 this is a global variable. The way you're using it means that this function will try fixing once, then it will not be reset to 0 after this signing... so all future buggy signings will be skipped.

This function feels like it's way too fancy, and also not really named what it is. I *\think* all that we need is for the original code to be the same but like this:

const sigOpts = {
  msg: msgParam,
  order: ['deviceKeyProxy', 'noop'],
}

const signature = await entropy.signWithAdaptersInOrder(sigOpt)
  .catch(err => {
    // HACK: sometimes order is reversed D:
    sigOpts.order.reverse()
    return entropy.signWithAdaptersInOrder(sigOpt)
  })

Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE this code assumes signWithAdaptersInOrder DOES NOT MUTATE the input... that would need checking.
We should not write functions which mutate inputs though 💀

Copy link
Contributor

Choose a reason for hiding this comment

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

don't see any mutations, should be fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔥 this is a global variable. The way you're using it means that this function will try fixing once, then it will not be reset to 0 after this signing... so all future buggy signings will be skipped.

This function feels like it's way too fancy, and also not really named what it is. I *\think* all that we need is for the original code to be the same but like this:

const sigOpts = {

  msg: msgParam,

  order: ['deviceKeyProxy', 'noop'],

}



const signature = await entropy.signWithAdaptersInOrder(sigOpt)

  .catch(err => {

    // HACK: sometimes order is reversed D:

    sigOpts.order.reverse()

    return entropy.signWithAdaptersInOrder(sigOpt)

  })

We could do this way of catching the error, but we still need to keep track of the attempts made to signing and handle accordingly. I find the try..catch to be cleaner, thoughts?

Copy link
Contributor

@mixmix mixmix Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  • try / catch is the same as promise.catch 🤷 (don't mind)
  • your global variable is still a liability - if multiple people are calling this function your global variable becomes a race condition

You need to change this to not use it - either the way I suggested, or pass the attempts count as an argument

async function signWithAdaptersInOrder (entropy, signingData, attempts = 0) {
  if (attempts > 1) throw Error('NOPE')
  // ...

  const newSignData = { /* ... */ } // shuffle
  return signWithAdaptersInOrder(entropy, newSignData, attempts + 1)
}


async function signWithAdaptersInOrder (entropy, signingData?: { msg: { msg: string }, order: string[] }) {
try {
const messageQuestion = {
type: 'list',
name: 'messageAction',
message: 'Please choose how you would like to input your message to sign:',
choices: [
'Text Input',
// 'From a File',
],
}
const userInputQuestion = {
type: "editor",
name: "userInput",
message: "Enter the message you wish to sign (this will open your default editor):",
}
// const pathToFileQuestion = {
// type: 'input',
// name: 'pathToFile',
// message: 'Enter the path to the file you wish to sign:',
rh0delta marked this conversation as resolved.
Show resolved Hide resolved
// }
let messageAction
if (!msg) {
({ messageAction } = await inquirer.prompt([messageQuestion]))
switch (messageAction) {
case 'Text Input': {
const { userInput } = await inquirer.prompt([userInputQuestion])
msg = Buffer.from(userInput).toString('hex')
break
}
// case 'From a File': {
// break
// }
rh0delta marked this conversation as resolved.
Show resolved Hide resolved
default: {
console.error('Unsupported Action')
return
}
}
}
debug('msg', msg);
const msgParam = { msg }
if (!signingData) {
signingData = {
msg: msgParam,
order: ['deviceKeyProxy', 'noop'],
}
}
const signature = await entropy.signWithAdaptersInOrder(signingData)
const signatureHexString = u8aToHex(signature)
// Resetting signingAttempts on success
signingAttempts = 0
print('signature:', signatureHexString)
print('verifying key:', entropy.signingManager.verifyingKey)
} catch (error) {
signingAttempts++
const { message } = error
// See https://github.com/entropyxyz/sdk/issues/367 for reasoning behind adding this retry mechanism
if (message.includes('Invalid Signer') || message.includes('Invalid Signer in Signing group')) {
if (signingAttempts <= 1) {
const msgParam = { msg }
signingData = {
msg: msgParam,
order: ['noop', 'deviceKeyProxy']
}
// Recursively retries signing with a reverse order in the subgroups list
await signWithAdaptersInOrder(entropy, signingData)
}
}
console.error(message)
return
}
}

export async function sign ({ accounts, endpoints, selectedAccount: selectedAccountAddress }, options) {
const endpoint = endpoints[options.ENDPOINT]
const actionChoice = await inquirer.prompt([
Expand Down Expand Up @@ -51,49 +128,7 @@ export async function sign ({ accounts, endpoints, selectedAccount: selectedAcco
// return
// }
case 'Sign With Adapter': {
const messageQuestion = {
type: 'list',
name: 'messageAction',
message: 'Please choose how you would like to input your message to sign:',
choices: [
'Text Input',
// 'From a File',
],
}
const userInputQuestion = {
type: "editor",
name: "userInput",
message: "Enter the message you wish to sign (this will open your default editor):",
}
// const pathToFileQuestion = {
// type: 'input',
// name: 'pathToFile',
// message: 'Enter the path to the file you wish to sign:',
// }
const { messageAction } = await inquirer.prompt([messageQuestion])
let msg: string
switch (messageAction) {
case 'Text Input': {
const { userInput } = await inquirer.prompt([userInputQuestion])
msg = Buffer.from(userInput).toString('hex')
break
}
// case 'From a File': {
// break
// }
default: {
console.error('Unsupported Action')
return
}
}
debug('msg', msg);
const msgParam = { msg }
const signature = await entropy.signWithAdaptersInOrder({
msg: msgParam,
order: ['deviceKeyProxy', 'noop'],
})

print('signature:', signature)
await signWithAdaptersInOrder(entropy)
return
}
case 'Exit to Main Menu':
Expand Down