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.

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

Construct Transaction API #211

Merged
merged 8 commits into from
Jan 13, 2025
Merged

Construct Transaction API #211

merged 8 commits into from
Jan 13, 2025

Conversation

n8maninger
Copy link
Member

@n8maninger n8maninger commented Jan 4, 2025

Adds two new endpoints to construct transactions. This combines and simplifies the existing fund flow for sending siacoin and siafund transactions.

Requires #210

@n8maninger n8maninger changed the title Construct txns Add Construct Transaction API Jan 4, 2025
@n8maninger n8maninger changed the title Add Construct Transaction API Construct Transaction API Jan 4, 2025
@n8maninger n8maninger changed the base branch from master to nate/unify-siacoin-select January 4, 2025 04:47
@lukechampine
Copy link
Member

lukechampine commented Jan 5, 2025

Definitely not safe to blindly sign whatever hashes a server gives you. 😬 This is a good illustration of how, strictly speaking, it's not possession of private keys that matters, but control.

Clients need to know what they're signing, which means they need to compute the hash. And they can't compute the hash blindly either (i.e. call BLAKE2b on a buffer returned by the server) for the same reason. At best, the server could return a buffer with preallocated space for the client to write the SiacoinOutputs into. But that's still vulnerable: the server could trick the client by having it write the outputs into ArbitraryData, while the actual SiacoinOutputs are malicious. So the client needs to be able to parse transactions. As far as I can tell, this is unavoidable.

IMO, any client should have the ability to parse transactions anyway (so that it can display them to the user), in addition to managing private keys and hashing/signing.

@n8maninger
Copy link
Member Author

n8maninger commented Jan 5, 2025

Definitely not safe to blindly sign whatever hashes a server gives you. 😬 This is a good illustration of how, strictly speaking, it's not possession of private keys that matters, but control.

You’re absolutely right that blindly signing hashes from an untrusted server is a security risk. However, this API is intended for trusted scenarios where the user controls both the client and the server.

IMO, any client should have the ability to parse transactions anyway (so that it can display them to the user), in addition to managing private keys and hashing/signing.

Requiring an implementation of full transaction parsing and sighash calculation in every possible language is really poor devex. Displaying a JSON object in a UI is relatively straightforward, implementing full sighash calculation is much more complex. For most languages, ed25519 libraries are available, but Sia specific transaction hashing logic isn’t.

The goal is to provide a minimal, generic, and language-agnostic flow for developers to interact with that balances usability and security for trusted implementations. For users that need maximum security and control, they still have the existing fund flow and can calculate the sig hash manually in Go or Rust.

I'm sure we can come up with a flow that is secure enough for daily use without requiring developers to dig into low-level Sia specific code unless they want to. Maybe it's as simple as adding additional steps so the user has to pass the transaction back to the server and can inspect it rather than caching it server-side?

@n8maninger n8maninger self-assigned this Jan 5, 2025
@n8maninger n8maninger removed the request for review from lukechampine January 5, 2025 18:05
@n8maninger n8maninger added this to the v1.0.0 milestone Jan 5, 2025
Base automatically changed from nate/unify-siacoin-select to master January 9, 2025 13:25
@n8maninger n8maninger force-pushed the nate/construct-txns branch from 782f0c9 to eab78f8 Compare January 9, 2025 17:43
@n8maninger n8maninger force-pushed the nate/construct-txns branch from eab78f8 to f122609 Compare January 9, 2025 17:44
@n8maninger n8maninger marked this pull request as ready for review January 9, 2025 17:44
@n8maninger
Copy link
Member Author

n8maninger commented Jan 9, 2025

@lukechampine I removed the signing flow from this. Now it just builds a transaction and returns it for manual signing.

api/server.go Outdated Show resolved Hide resolved
api/server.go Show resolved Hide resolved
api/server.go Show resolved Hide resolved
api/server.go Outdated Show resolved Hide resolved
api/server.go Show resolved Hide resolved
api/api_test.go Show resolved Hide resolved
api/server.go Show resolved Hide resolved
api/server.go Show resolved Hide resolved
api/server.go Show resolved Hide resolved
.changeset/add_transaction_construction_api.md Outdated Show resolved Hide resolved
api/client.go Outdated Show resolved Hide resolved
api/client.go Outdated Show resolved Hide resolved
.changeset/add_transaction_construction_api.md Outdated Show resolved Hide resolved
api/server.go Show resolved Hide resolved
api/api_test.go Show resolved Hide resolved
peterjan
peterjan previously approved these changes Jan 13, 2025
Copy link
Member

@peterjan peterjan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

ChrisSchinnerl
ChrisSchinnerl previously approved these changes Jan 13, 2025
@n8maninger n8maninger dismissed stale reviews from peterjan and ChrisSchinnerl via b729692 January 13, 2025 16:11
@n8maninger n8maninger merged commit 780c780 into master Jan 13, 2025
9 checks passed
@n8maninger n8maninger deleted the nate/construct-txns branch January 13, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants