-
Notifications
You must be signed in to change notification settings - Fork 41
scripts: Add create_load_fund_wallet() make sure bitcoind wallet is setup correctly #91
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
scripts: Add create_load_fund_wallet() make sure bitcoind wallet is setup correctly #91
Conversation
…etup correctly before executing test cases remove -rpcport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| else | ||
| echo "Creating new wallet: $wallet_name" | ||
| $bitcoin_cli createwallet "$wallet_name" &>/dev/null | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wallet detection uses wrong command for existing wallets
Medium Severity
The listwallets command only returns currently loaded wallets, not wallets that exist on disk. When getwalletinfo fails and the code checks listwallets, a wallet that exists on disk but isn't loaded won't appear in the list. This causes the code to incorrectly call createwallet instead of loadwallet. The create operation will fail silently (due to &>/dev/null) because the wallet already exists, leaving the wallet in an unloaded state. To detect wallets on disk, listwalletdir is needed, or the code could try loadwallet first and fall back to createwallet on failure.
|
Thank you Chris! This should be fixed now. Please let us know if you’re still running into any issues. |
|
Yeah it looks like its addressed in a combination of Another suggestion
I can put this on my backlog of things to do if there is appetite for this internally at lightspark. Another q: Can you explain to me why sometimes the go is managing bitcoind state (00343a3) and sometimes the script is? Seems like it may be worth just moving all bitcoind state handling logic into the go test harness. Perhaps there isn't a clean separation of concerns between a "localized" setup and unit/integration testing the application - might be worth thinking about this as these will likely become a bigger problem as spark becomes more successful 🤷♂️ |
|
Thanks Chris for all the recommendations! We will file these tickets internally and working on those. The commit you pointed out is actually the branch commit which is reverted in later commits without squashing all commits. But yes we will try to manage the bitcoind state by script from now on, and will move the legacy testing setup from go side to script side. |
Just to be clear, both contexts can make sense. For instance, your test suite should be able to call Hopefully you guys can move to segregating the testing environment and running the app locally which would make this problem go away, admittedly that will take some work though and I can understand why that isn't the highest of priorities. Thanks for the responses! |
fixes parts of #79
This PR adds functionality to
./run-everything.shtospark-walletspark-wallet's balanceThis gets more unit tests passing locally for me (unfortunately, i'm running into more instances of #90 ). I'll eventually get around to putting this into #71 to see what tests look like on CI. I think this can be merged independently though to make lives of non-minikube users easier.
Note
Ensures a usable regtest wallet is available immediately after starting bitcoind.
create_load_fund_walletto create/loadspark-wallet, check balance, andgeneratetoaddress101 blocks if balance < 1 BTCsleepto allow startup)bitcoin_regtest.confviaparse_bitcoin_configWritten by Cursor Bugbot for commit 6559e71. This will update automatically on new commits. Configure here.