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

wallet-http: put send transaction endpoints behind fund locks. #845

Merged
merged 3 commits into from
Aug 31, 2023

Conversation

nodech
Copy link
Contributor

@nodech nodech commented Aug 28, 2023

HTTP Endpoints that are responsible to directly sending transactions to the mempool (except send tx) - did not use fund lock to queue the requests to the wallet. What ends up happening if you are running automated scripts or send multiple requests at once - coins wont get marked as spent before another set of coins are selected. Resulting in conflicting transactions.

This makes sure every send* transaction:

  • POST /wallet/:id/open
  • POST /wallet/:id/bid
  • POST /wallet/:id/reveal
  • POST /wallet/:id/redeem
  • POST /wallet/:id/update
  • POST /wallet/:id/renewal
  • POST /wallet/:id/transfer
  • POST /wallet/:id/cancel
  • POST /wallet/:id/finalize
  • POST /wallet/:id/revoke

Uses fundlock until the coin is added to the txdb, before processing next HTTP request.

Hard Fee

Expose hardFee as an option to the HTTP. This can help users to specify the exact FEE they want to pay for transaction.

NOTE About TODOs.

// TODO: Add create TX with locks for used Coins and/or
// adds to the pending list.

When creating transaction (w/o sending), you run into the same issue. If you create 2 transactions at once, chances are they will use the same coins to fund them. There are two solution with existing tools for this:

  • If createTX is SIGNED, we can add those transactions to the txdb w/o sending to the mempool.
  • If createTX is NOT SIGNED, we can LOCK those coins, to ensure next createTX does not use the same coins to fund next one.
    This can be done by introducing new flags: insert - if signed but no broadcast - we can look at this flag and insert into txdb. And another flag: lock - if we are not signed (and of course that also means no broadcast), we lock the coins in non-permanently.

// TODO: Add abort signal to close when request closes.

Big wallets, with lots of coins, or big transactions that needs lots of coins can take a some time to craft transactions before broadcasting. We get compounding effect when we use fundLocks where multiple requests are also queued.
So allowing new request parameter abortOnClose can work by making sure that transaction creation is done before request closes. If request closes and transaction already was being sent, it will do nothing.

@coveralls
Copy link

coveralls commented Aug 28, 2023

Coverage Status

coverage: 68.628% (-0.02%) from 68.65% when pulling e49a8fc on nodech:http-races into 61ae19c on handshake-org:master.

test/wallet-http-test.js Outdated Show resolved Hide resolved
test/wallet-http-test.js Outdated Show resolved Hide resolved

// spend all money for now.
await rcwallet1.send({
subtractFee: true,
Copy link
Member

Choose a reason for hiding this comment

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

is passphrase needed here for consistency?

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 works because the wallet is unlocked at this stage. (Wallet is locked in the afterEach)
I will just add comment.

@rithvikvibhu
Copy link
Member

  • The test expectedly fails on master with double spend errors 👍
  • searched for all wallet.send() calls, none passed passphrase as 2nd arg
    LGTM

@nodech nodech merged commit 7aeb668 into handshake-org:master Aug 31, 2023
@nodech nodech deleted the http-races branch August 31, 2023 09:41
@nodech nodech added this to the hsd 7.0.0 milestone Nov 3, 2023
@nodech nodech added bug general - Something isn't working wallet-http part of the codebase patch Release version labels Nov 3, 2023
@nodech nodech mentioned this pull request Nov 29, 2024
@nodech nodech mentioned this pull request Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug general - Something isn't working patch Release version wallet-http part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants