Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

signet testing: transaction that was evicted from mempool and abandoned by sender wallet showed up as a new unconfirmed txn on sparrow recepient #74

Open
7hacker opened this issue Mar 11, 2021 · 12 comments
Labels
bug Something isn't working low Low priority upstream Requires changes in an upstream project

Comments

@7hacker
Copy link

7hacker commented Mar 11, 2021

  1. Start sparrow with -n signet -l DEBUG, there is currently one unconfirmed txn that is waiting indefinitely in the mempool since its fee is lower than allowed. This is seen as "Unconfirmed"

Screen Shot 2021-03-10 at 11 12 06 PM

  1. shutdown the signet bitcoind, disconnect sparrow from the bitcoin core
  2. delete mempool.dat and restart signet bitcoind with -walletbroadcast=0
  3. run abandontransaction on the sender's wallet to abandon the transaction completely
  4. the mempool is now empty with 0 txns
~/code/bitcoin/src (master) $ ./bitcoin-cli -datadir=signet-custom -rpcwallet=test getmempoolinfo
{
  "loaded": true,
  "size": 0,
  "bytes": 0,
  "usage": 0,
  "total_fee": 0.00000000,
  "maxmempool": 300000000,
  "mempoolminfee": 0.00001000,
  "minrelaytxfee": 0.00001000,
  "unbroadcastcount": 0
}
  1. sparrow's rpc calls logging correctly show that the prev txid is not found anymore
DEBUG bitcoincore_rpc                   > JSON-RPC request: getmempoolentry ["bbe825871e49a284d86f323238415d7d105c9f56a9bb9ed0c1a36606ad1903f8"]
 DEBUG bitcoincore_rpc                   > JSON-RPC error for getmempoolentry: RpcError { code: -5, message: "Transaction not in mempool", data: None }
 DEBUG bwt::indexer                      > failed fetching mempool entry for bbe825871e49a284d86f323238415d7d105c9f56a9bb9ed0c1a36606ad1903f8: JSON-RPC error: RPC error response: RpcError { code: -5, message: "Transaction not in mempool", data: None }


  1. But sparrow's UI / client showed me a notification of a new unconfirmed txn that is incoming , and the below screenshot showing an 'unconfirmed parent'

Screen Shot 2021-03-11 at 12 45 17 AM

I'm not sure if this is correct but assuming that no txn would ever come then this entry would be unncessary?

@craigraw
Copy link
Collaborator

Interesting test! On the one hand, it would be good to reflect what's in the user's mempool, and on the other, in the general case broadcasting a transaction means it can never truly be considered lost.

What happens when you refresh the wallet in this state (View menu)?

@7hacker
Copy link
Author

7hacker commented Mar 12, 2021

  1. View menu -> refresh wallet
  2. Sparrow wallet disconnects from signetd bitcoin
  3. Use the slider in bottom right to reconnect
  4. Sparrow UI stalls, becomes unresponsive, and logs show:
 DEBUG bitcoincore_rpc                   > JSON-RPC request: getwalletinfo []
2021-03-11 20:15:14,281 ERROR Error in version check address
com.sparrowwallet.drongo.address.InvalidAddressException: Could not parse invalid address 1LiJx1HQ49L2LzhBwbgwXdHiGodvPg5YaV
        at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.address.Address.fromString(Address.java:132)
        at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.address.Address.fromString(Address.java:67)
        at com.sparrowwallet.sparrow@1.2.0/com.sparrowwallet.sparrow.net.VersionCheckService.verifySignature(VersionCheckService.java:68)
        at com.sparrowwallet.sparrow@1.2.0/com.sparrowwallet.sparrow.net.VersionCheckService$1.call(VersionCheckService.java:35)
        at com.sparrowwallet.sparrow@1.2.0/com.sparrowwallet.sparrow.net.VersionCheckService$1.call(VersionCheckService.java:31)
        at javafx.graphics/javafx.concurrent.Task$TaskCallable.call(Task.java:1425)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$6(Service.java:725)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
        at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$7(Service.java:724)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
        at java.base/java.lang.Thread.run(Thread.java:832)
<===========--> 88% EXECUTING [3m 8s]
> :run
  1. Try to recover by running:
    ./bitcoin-cli -datadir=signet-custom unloadwallet sparrow

Which also fails to return

  1. Kill sparrow process, and restart
  2. This time connection establishes but UI continues to show the Unconfirmed parent as well as the following logs from the console
 DEBUG bwt::indexer                      > failed fetching mempool entry for bbe825871e49a284d86f323238415d7d105c9f56a9bb9ed0c1a36606ad1903f8: JSON-RPC error: RPC error response: RpcError { code: -5, message: "Transaction not in mempool", data: None }
2021-03-11 20:18:54,885 ERROR Error in version check address
com.sparrowwallet.drongo.address.InvalidAddressException: Could not parse invalid address 1LiJx1HQ49L2LzhBwbgwXdHiGodvPg5YaV
        at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.address.Address.fromString(Address.java:132)
        at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.address.Address.fromString(Address.java:67)
        at com.sparrowwallet.sparrow@1.2.0/com.sparrowwallet.sparrow.net.VersionCheckService.verifySignature(VersionCheckService.java:68)
        at com.sparrowwallet.sparrow@1.2.0/com.sparrowwallet.sparrow.net.VersionCheckService$1.call(VersionCheckService.java:35)
        at com.sparrowwallet.sparrow@1.2.0/com.sparrowwallet.sparrow.net.VersionCheckService$1.call(VersionCheckService.java:31)
        at javafx.graphics/javafx.concurrent.Task$TaskCallable.call(Task.java:1425)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$6(Service.java:725)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
        at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$7(Service.java:724)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
        at java.base/java.lang.Thread.run(Thread.java:832)
 DEBUG bitcoincore_rpc                   > JSON-RPC request: getblockhash [1733]

@craigraw
Copy link
Collaborator

The InvalidAddressException is unrelated - it's just caused by Sparrow failing to parse a mainnet address with a signet configuration. Fixed in 3e287bf.

If Sparrow is disconnecting on a wallet refresh, then it seems bwt (the library Sparrow is using to connect to the node) is experiencing a terminal error. Are there any logs that might indicate why this happening? bwt's debug logging is included inside Sparrow's logging, so it should be present. The bwt logs look similar to the first line in the log excerpts above.

@7hacker
Copy link
Author

7hacker commented Mar 12, 2021

I think I know whats happening here. Just to be sure i recreated the last result again, but this time captured the full log which is attached below. Reading the logs top to bottom should ideally reflect the events described below but if i missed something let me know. Also curious if you find something additional too

  1. Start sparrow wallet with -n signet -l DEBUG
  2. Go to View->refresh wallet
  3. The bwt library doesn't perform a connection when the wallet is refreshed, because it hits this error that @shesek mentioned in Issue: sparrow cant connect to signet node running on localhost #67 .

dev.bwt.libbwt.BwtException: JSON-RPC error: RPC error response: RpcError { code: -35, message: "Wallet file verification failed. Refusing to load database. Data file \'/Users/nirmal/code/bitcoin/src/signet-custom/signet/wallets/sparrow/wallet.dat\' is already loaded.", data: None }

  1. To get around this, I perform the workaround referenced above which is to unload the sparrow wallet
  2. Then as mentioned above in point 5 this fails to return, but actually it does return after a very long time. This time i waited :)
  3. And finally when it does return, sparrow shows the lost transaction as previously , an 'Unconfirmed Parent'
  4. This time i examine the transaction by clicking the magnifying button and the RPC mempool failure is also logged in sparrow.log

Sparrow.log
sparrow.log

@7hacker
Copy link
Author

7hacker commented Mar 12, 2021

@craigraw

Its possible that this issue concerns a case that is rare/improbable? As you mentioned previously, that broadcasted transactions are not considered 'lost' . However I am puzzled of why the entry switched from "Unconfirmed" to "Unconfirmed Parent". Should it continue staying "Unconfirmed"?

@craigraw
Copy link
Collaborator

I believe your thinking is correct - this is a rare/improbable use case, but nevertheless I have been suspicious for some time that there is a subtle bug causing the "Unconfirmed Parent" situation when it is not always correct. That bug might be in bwt however - note that in the Electrum server protocol specification for blockchain.scripthash.get_mempool, the server notifies the client by returning -1 when some of the inputs of a mempool transaction are unconfirmed. Sparrow uses this information to add the "Unconfirmed Parent" label.

Although it should be present in the bwt logging too, may I suggest adding debug logging to SubscriptionService to see how bwt is following the protocol on this - strictly, it should notify the script hashes attached to the transaction to return them to their original states (prior to initiating the test). I should note that Sparrow does not handle that situation as you would expect - a bug in electrs means that Sparrow needs to ignore old script hash statuses, or be overwhelmed with unnecessary server queries after invalid notifications.

It would also be interesting to know if normal situation where a transaction has an unconfirmed parent are working correctly.

cc'ing @shesek (author of bwt)

@shesek
Copy link

shesek commented Mar 15, 2021

bwt should be handling transactions with unconfirmed parents correctly and report them with a height of -1, as of v0.1.4. Confirmation of the parent transactions should result in a status hash change and a notification.

If something still seems off on bwt's side, let me know and I'll look into this. It probably wouldn't hurt to add some tests around this at least.

Edit: It's not strictly related, but maybe also worth noting that bwt reports the effective feerate of transactions with unconfirmed parents, calculated as MIN(own_fee/own_vsize, (own_fee+ancestor_fee)/(own_vsize+ancestor_vsize)). It is the only Electrum server implementation that does it, which may result in some different/unexpected behaviors.

@7hacker
Copy link
Author

7hacker commented Mar 17, 2021

hi @craigraw @shesek thanks for the discussion, here's what I'll do as next steps: check the wallet behavior for an actual unconfirmed parent.

Meanwhile if there's a patch you'd like me to build with that will provide additional logs for Sparrow or BWT, I'm happy to re-run this flow and provide the data for further debugging

@craigraw
Copy link
Collaborator

Great - bwt's debug logging is also turned on when Sparrow's is.

@6102bitcoin 6102bitcoin added the bug Something isn't working label May 20, 2021
@6102bitcoin
Copy link
Collaborator

@7hacker did you find time to perform this test with an unconfirmed parent?

Outstanding Action: Identify whether problem still exists. Identify whether fix can be applied to bwt and if so talk to shesek.
Proposed Priority: Low

@7hacker
Copy link
Author

7hacker commented May 21, 2021

@6102bitcoin no i haven't had a chance to try this but I can spend some time on it. Do you have a suggestion of how to generate an unconfirmed parent? I'm using a custom signet here

@6102bitcoin
Copy link
Collaborator

I don't. @craigraw might know how

@6102bitcoin 6102bitcoin added the low Low priority label May 23, 2021
@6102bitcoin 6102bitcoin added the upstream Requires changes in an upstream project label Nov 21, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working low Low priority upstream Requires changes in an upstream project
Projects
None yet
Development

No branches or pull requests

4 participants