fix: p-460 bitcore message signature issue #2553

0xverin · 2024-03-05T08:28:26Z

Context

resolves:p-460

Root cause: bitcore uses the ECDSA signature algorithm, which uses a nonce to sign messages. This causes the signature (base64) to not be consistent. When decoding, we might need to manually pad it with a 0 byte.

For example, under normal the signature is:
IJyax3CRyGOCceIyoBDpH9a03dwi3SoRJ4N1RZh36FxARkDCpEmUgrHqIhE5H8THYF8TcE5HuH/9BkovbYmMSbk=
But sometimes:
H6FuCBQt/fjshZIym0BBgGjbykkqBWO6CisWA9tBDDMD/5iODJehfyUXX3MrM21l5dHSuG46KqXiFEA7X94apw==
This is a base64 encoding issue.

~~I tested it and adding a 0 can make it work normally:~~

await signer.sign(payloadStr)------ Uint8Array(65) [
    0,  32, 157, 209, 236, 186,  54,  14, 139,  90, 150,
   47, 194, 137,  67,  86,  82, 112, 120, 102,  63, 223,
  156, 127, 240, 243, 129, 151,  19,  58, 202,  25, 246,
  121,  56, 254, 247,  39, 203,  68, 213, 168, 236,  53,
  163,  69, 207, 182,   3, 156, 234, 203, 145, 236,  34,
  150,  18, 212, 121, 148,  33, 130, 253, 117, 127
]
Got response: {"value":"0x010400","do_watch":false,"status":"Ok"}

Updates:
The solution of adding 0 was wrong, will change the content of sig, causing it to be invalid when used. I updated it with a new way.

After 2000 cycles of testing, there were no issues with the do-while sign method:

Got response: {"value":"0x010400","do_watch":false,"status":"Ok"}
    ✔ check idGraph from sidechain storage before linking (2034579ms)

linear · 2024-03-05T08:28:29Z

P-460 lit-di-bitcoin-identity-test sometimes fails in CI

Kailai-Wang

Thanks for finding the root cause! 👍

Not sure if it's related to nonce tho (I haven't checked the lib impl and am not sure if the k is random or not) - maybe it's a sole consequence of having random btc key pairs each time.

Kailai-Wang · 2024-03-05T10:07:57Z

tee-worker/ts-tests/integration-tests/common/utils/crypto.ts

@@ -145,7 +145,11 @@ export class BitcoinSigner implements Signer {
        return new Promise((resolve, reject) => {
            if (isString(message)) {
                const sig = new bitcore.Message(message).sign(this.keypair);
-                resolve(bufferToU8a(Buffer.from(sig, 'base64')));
+                let buf = Buffer.from(sig, 'base64');
+                if (buf.length === 64) {


Is it possible to have more idiomatic code by using e.g. a fixed length buffer? Something like [u8; 65] in rust to make sure it's always 65 bytes 🤔

After testing, it is not just the error case where buffer.length === 64. In fact, there will also be cases where it equals 63 (not excluding other cases, 63 is the result of 1000 loops). Therefore, change to using do while to ensure getting u8aArray 65 sig.

1) Test Identity (bitcoin direct invocation) check idGraph from sidechain storage before linking: Error: createType(LitentryMultiSignature):: Enum(bitcoin):: Expected input with 65 bytes (520 bits), found 63 bytes

The way of creating a buffer seems to be impractical, as it will change the content of the signature, such as this way:

let buf = Buffer.from(sig, 'base64'); Buffer.alloc(65) buf.copy(resultBuf, 65 - buf.length);

Will get an invalid error:
Got response: {"value":"TGetterIsNotAuthorized","do_watch":false,"status":"Error"}

So my initial approach was also incorrect. The reason why the test passed is that when I console.log here, actually called the sign method. Due to the uncertainty of the signature result, after two console calls in front, the third call here happened to match the sig we used, so the test passed strangely.

BillyWooo

Thank you for finding the root cause.

0xverin · 2024-03-05T11:19:28Z

I haven't checked the lib impl and am not sure if the k is random or not

I checked:

0xverin · 2024-03-05T16:26:04Z

~~Please do not merge first, there is a small issue that needs to be confirmed.~~

Kailai-Wang

Btw what about bitcore.Message in identity-helper.ts?

Kailai-Wang · 2024-03-06T08:30:28Z

tee-worker/ts-tests/integration-tests/common/utils/crypto.ts

+                // Here we must use the format [u8,65],
+                // so we can only accept a signature that has been decoded from base64 and has a length of 65.
+                // Therefore, using a do -while loop to resign unnecessary signatures.We should not encounter an infinite loop situation, so no condition is set to break out of the loop.
+                do {


I somehow feel this isn't the right way, still.

A btc message signature should be 65 bytes - the encoded base64 array should be 88 bytes. This should be firm. If we get less than 65 bytes in decoding, I can only think of padding?

Let it run again and hope to get 65 bytes isn't the good practice IMO, as in reality you should be able to verify each btc signature the user generated, with arbitrary k

cc @kziemianek - maybe you know it as you've done some btc sig related impl

In my opinion it should always produce 65 bytes signature, even bitcore-lib developers assumes that.
If you try to use bitcode-lib api to recreate signature from Buffer it will fail:

const buffer = Buffer.from(sig, 'base64') Signature.fromCompact(buffer) Invalid Argument: Error: s must be 32 bytes Error at new NodeError (/home/kziemianek/projects/kziemianek/sig-test/node_modules/bitcore-lib/lib/errors/index.js:20:41) at Object.checkArgument (/home/kziemianek/projects/kziemianek/sig-test/node_modules/bitcore-lib/lib/util/preconditions.js:14:13) at Signature.fromCompact (/home/kziemianek/projects/kziemianek/sig-test/node_modules/bitcore-lib/lib/crypto/signature.js:53:5)

I debugged it for a while and it turns out the r value is sometimes 31 bytes long, which results in 64 (1+31+32) bytes signature (instead of 1+32+32). Above error says s must be 32 bytes but the root cause is still r being 31 bytes long.

It happens ocassionally so it's really low probability of happeining twice in the row.

So to sum up, I see two options:

switch to other js bitcoin lib (https://www.npmjs.com/package/bitcoinjs-lib maybe ?)

implement a workaround (try to generate new signature untill it has 65 bytes) and wait for the potential fix in bitcore-lib - but before that happening it needs to be reported, confirmed and released.

Thanks for your summary @kziemianek , I will switch to bitcoinjs for implementation. Btw, I have looked at the SDK of unisat-wallet and switched to the version they use, but there is still the same problem.

cc @jonalvarezz - in case you have some experience when implementing SDK..

The new bitcoinjs-lib was tested twice in 2000 loops with no signature decoding issues. I also tested identity-helper several times, and there was no problem:

Got response: {"value":"0x010400","do_watch":false,"status":"Ok"} ✔ check idGraph from sidechain storage before linking (2012011ms)

cc @kziemianek @Kailai-Wang

jumping in late, but I hope it helps.

Here is what we do on the SDK for Bitcoin in a nutshell:

The hex challenge code should be signed without the 0x prefix. Only relevant if signing in hex/string format. We were using raw uint signing before, but I remember we had to change this because of bitcoin/ unisat.

https://github.com/litentry/identity-hub/blob/develop/libs/enclave/src/lib/requests/link-identity.request.ts#L63

The produced signature is base64 decoded and the 0x prefix put back. Otherwise, LitentryValidationData fails at encoding.

https://github.com/litentry/identity-hub/blob/develop/libs/enclave/src/lib/type-creators/validation-data.ts#L145-L153

thanks @jonalvarezz I actually looked at the code on idehub, and in the Bitcoin signature part, mainly use unisat-wallet. This wallet uses their own integrated unisat-sdk, and you will find that they also use bitcore-lib and bitcoinjs-lib.So, essentially there is no difference, I guess.:p

tee-worker/ts-tests/integration-tests/common/utils/vc-helper.ts

…entry/litentry-parachain into fix-bitcoin-identity-failure

tee-worker/ts-tests/integration-tests/common/common-types.ts

tee-worker/ts-tests/integration-tests/di_bitcoin_identity.test.ts

tee-worker/ts-tests/integration-tests/common/helpers.ts

tee-worker/ts-tests/integration-tests/common/utils/crypto.ts

Kailai-Wang

For me it's ready to be merged

0xverin added 3 commits March 5, 2024 03:44

debug:check idGraph with aliceBitcoinIdentity

cbad76b

fmt

5991e73

fix bitcore signmessage issue

626eead

remove log && uncomment

d87def6

0xverin requested review from Traf333, Kailai-Wang, BillyWooo and grumpygreenguy March 5, 2024 08:49

Kailai-Wang reviewed Mar 5, 2024

View reviewed changes

BillyWooo approved these changes Mar 5, 2024

View reviewed changes

modify bitcore sign message logic

7877432

0xverin force-pushed the fix-bitcoin-identity-failure branch from d7efdf0 to 7877432 Compare March 6, 2024 04:27

Kailai-Wang requested changes Mar 6, 2024

View reviewed changes

Kailai-Wang requested a review from kziemianek March 6, 2024 08:30

0xverin added 6 commits March 6, 2024 09:01

Merge branch 'dev' into fix-bitcoin-identity-failure

b83eda6

switch to bitcoinjs lib

7dbacb9

del unused deps

5cc0949

try 2000 tests

f69763a

fmt

7af92d0

complete tests

50da585

0xverin force-pushed the fix-bitcoin-identity-failure branch from de78d82 to 50da585 Compare March 7, 2024 05:12

0xverin requested a review from Kailai-Wang March 7, 2024 05:12

0xverin and others added 4 commits March 7, 2024 05:16

fmt one more

9cd3fcc

pnpm-lock file

2234099

pass ci

8812d3c

Merge branch 'dev' into fix-bitcoin-identity-failure

09d6599

kziemianek reviewed Mar 7, 2024

View reviewed changes

tee-worker/ts-tests/integration-tests/common/utils/vc-helper.ts Outdated Show resolved Hide resolved

0xverin added 4 commits March 7, 2024 13:16

Uncomment karat_dao test

358a86c

Merge branch 'fix-bitcoin-identity-failure' of https://github.com/lit…

56459db

…entry/litentry-parachain into fix-bitcoin-identity-failure

rollback handleVcEvents

accc452

uncomment one more

a0b2153

kziemianek reviewed Mar 7, 2024

View reviewed changes

tee-worker/ts-tests/integration-tests/common/common-types.ts Show resolved Hide resolved

kziemianek reviewed Mar 7, 2024

View reviewed changes

tee-worker/ts-tests/integration-tests/di_bitcoin_identity.test.ts Outdated Show resolved Hide resolved

tee-worker/ts-tests/integration-tests/common/helpers.ts Outdated Show resolved Hide resolved

tee-worker/ts-tests/integration-tests/common/utils/crypto.ts Outdated Show resolved Hide resolved

fix comments

1def4a5

Kailai-Wang approved these changes Mar 8, 2024

View reviewed changes

0xverin merged commit 744bc85 into dev Mar 8, 2024
31 checks passed

0xverin deleted the fix-bitcoin-identity-failure branch March 8, 2024 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: p-460 bitcore message signature issue #2553

fix: p-460 bitcore message signature issue #2553

0xverin commented Mar 5, 2024 •

edited

linear bot commented Mar 5, 2024

Kailai-Wang left a comment

Kailai-Wang Mar 5, 2024

0xverin Mar 6, 2024

BillyWooo left a comment

0xverin commented Mar 5, 2024 •

edited

0xverin commented Mar 5, 2024 •

edited

Kailai-Wang left a comment

Kailai-Wang Mar 6, 2024

kziemianek Mar 6, 2024

kziemianek Mar 6, 2024 •

edited

0xverin Mar 6, 2024

Kailai-Wang Mar 7, 2024

0xverin Mar 7, 2024

jonalvarezz Mar 7, 2024 •

edited

0xverin Mar 8, 2024

Kailai-Wang left a comment

fix: p-460 bitcore message signature issue #2553

fix: p-460 bitcore message signature issue #2553

Conversation

0xverin commented Mar 5, 2024 • edited

Context

linear bot commented Mar 5, 2024

Kailai-Wang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyWooo left a comment

Choose a reason for hiding this comment

0xverin commented Mar 5, 2024 • edited

0xverin commented Mar 5, 2024 • edited

Kailai-Wang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kziemianek Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonalvarezz Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kailai-Wang left a comment

Choose a reason for hiding this comment

0xverin commented Mar 5, 2024 •

edited

0xverin commented Mar 5, 2024 •

edited

0xverin commented Mar 5, 2024 •

edited

kziemianek Mar 6, 2024 •

edited

jonalvarezz Mar 7, 2024 •

edited