fix: resolve string treated as buffer without explicit conversion #432
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where a string was being passed to PeerWallet.blake3() without explicit conversion to a buffer, which could cause runtime errors or unexpected behavior depending on the API's requirements.
- Removes unnecessary address conversion by passing
message.addressdirectly todecodeBech32m() - Adds explicit buffer conversion using
b4a.from()before hashing withblake3()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default: | ||
| const addressString = bufferToAddress(message.address, this.#config.addressPrefix); | ||
| publicKey = PeerWallet.decodeBech32m(addressString); | ||
| publicKey = PeerWallet.decodeBech32m(message.address); |
There was a problem hiding this comment.
This looks ok and makes sense
| const messageWithoutSig = { ...message }; | ||
| delete messageWithoutSig.sig; | ||
| const hash = await PeerWallet.blake3(JSON.stringify(messageWithoutSig)); | ||
| const hashInput = b4a.from(JSON.stringify(messageWithoutSig), 'utf8'); |
There was a problem hiding this comment.
We can start the discussion here. In src/core/network/messaging/handlers/GetRequestHandler.js, the message is built without UTF‑8 encoding, while here you already apply that encoding, which will cause inconsistency.
- The question is: it must have been working in the version where you hadn’t yet converted messageWithoutSig to binary with encoding. If it didn’t work, then test it. But I believe that it was working, nodes find each other because, if they do, it means that the signatures are valid.
- What if some nodes use b4a.from(JSON.stringify(messageWithoutSig), 'utf8') and others still use the previous version await PeerWallet.blake3(JSON.stringify(messageWithoutSig));? The messages will become incompatible.
There was a problem hiding this comment.
Btw how did you localize this? Did you notice a bug?
There was a problem hiding this comment.
Yes. Lots of error messages like
NetworkMessages: Failed to handle incoming message: Failed to route message: Invalid input: must be a Buffer or Uint8Array. Pubkey of requester is ...I investigated and found out these parts in the code were the source of the problem
There was a problem hiding this comment.
Aswering 2:
This behavior was introduced with the last commit, about changing the source of blake3 from crypto.js to PeerWallet.
The issue is that crypto.js did this conversion internally if the input was a string:
// crypto.js (deleted)
export async function blake3Hash(input, hashLength = 32) {
if (typeof input === "string") {
input = b4a.from(input, "utf8");
}
const hashBytes = await blake3(input, hashLength);
return b4a.from(hashBytes);
}In the trac-crypto-api, and thus, PeerWallet, we don't do this conversion implicitly. We request a buffer as input by design.
So I don't believe this will cause inconsistencies because, essentially, the logic is the same.
No description provided.