-
Notifications
You must be signed in to change notification settings - Fork 138
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
Bitcoind for onchain #317
Bitcoind for onchain #317
Conversation
…(TODO: estimateSmartFee is not working yet)
@jotapea please pay attention to your commit messages we are trying to follow this guide for standard commit messages (first line 50 characters). And the |
@bodymindarts Will do, thanks for the feedback! |
This reverts commit e2a4b73.
- Encapsulates variation between onchain clients - LndOnChainClient class implements the original lnd-onchain calls - This class may be temporary, but it is currently useful to switch between lnd and bitcoind in one line
- TODO: getEstimatedFee method is not working yet - The tests start to fail...
@@ -62,6 +62,7 @@ afterAll(async () => { | |||
|
|||
const amount = 10040 // sats | |||
|
|||
// Starts to fail from here... |
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.
if it is a timeout fail is because waitUntilBlockHeight. Basically in lnd the "pruning channel graph" is taking more than the timeout in jest.
- you can check docker logs to verify that this is the case
- increase timeout in jest setup https://github.com/GaloyMoney/galoy/blob/main/test/jest.setup.js
also you can check if adding routing.assumechanvalid=true to lnd.conf can help. I have not fully tested but you can give it a try
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.
Thanks for this, modifying the timeout has allowed me to retry the tests when these show the following error:
FAIL test/integration/02a-fund-bitcoind.spec.ts (36.247 s)
✓ funds bitcoind wallet (3697 ms)
✕ funds outside lnd node (30268 ms)
● funds outside lnd node
thrown: "Exceeded timeout of 30000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."
66 | })
67 |
> 68 | it("funds outside lnd node", async () => {
But this seems to be "unrelated?" to the the main failure that the tests are showing, which is that the balance sheet is no longer balanced.
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.
@jotapea FYI tests are NOT idempotent so you always have to run make reset-deps
between runs.
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.
Yes I'm running them with 'make integration' the first time and then 'make reset-integration' the rest of the times.
abstract getTxnFee(id: string): Promise<number> | ||
} | ||
|
||
class LndOnChainClient extends PayOnChainClient { |
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.
is a highly coupled class with getActiveOnchainLnd. Maybe is better pass the lnd client as parametter (the same for BitcoindClient)
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.
The main purpose of the classes at the moment is for ease of development purposes. After the bitcoind client works as a replacement to the lnd client, then we could reconsider the purpose and structure of these classes.
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.
but you have only one instance (line 68) and in the same file
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.
is this still relevant @dolcalmi ?
@@ -217,16 +323,17 @@ export const OnChainMixin = (superclass) => | |||
throw new TransactionRestrictedError(error, { logger: onchainLogger }) | |||
} | |||
|
|||
const { lnd } = getActiveOnchainLnd() | |||
// const { lnd } = getActiveOnchainLnd() |
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.
@dolcalmi Notice that this is the same call that is now in line 90 (this.client = getActiveOnchainLnd().lnd), so it should work just like it used to.
The equivalent test for the default wallet passes
@@ -65,6 +63,22 @@ it("funds bitcoind wallet", async () => { | |||
expect(balance).toBe(initialBitcoinWalletBalance + blockReward * numOfBlock) | |||
}) | |||
|
|||
// This new test fails afterFinished: checkIsBalanced() |
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.
All tests still pass after I renamed the default wallet from "" (unnamed) to "default". But adding the test to create a second wallet ("hot") fails just after creation. Made a comment of this failure: https://github.com/GaloyMoney/galoy/pull/317/files#diff-67ab19f16c472b30989f27f5755650158189756bbf68874767a0ff7196c50a05
@nicolasburtey @krtk6160 how should I proceed? Why is the "default" wallet bypassed for the balance sheet tests?
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.
not sure if you can change the default wallet. The current production environment use "" as default wallet
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.
@jotapea I will need to dig deeper to understand why it's failing but some feedback on top of my head that could be relevant:
- since bitcoind 0.20 (or 0.21?) the behavior of bitcoind changed in regards to defauilt wallet and now no default wallet is being created automatically. the code has been udpated to accomodate this but IIRC it was a bit hacky
- we currently only use the bitcoin wallet in production is for the multisig that is created by specter. the other use of bitcoind is currently only for the test (maybe we should have 2 bitcoind instance to make this more explicit... one that is for the wallet we use, and another bitcoind that represents the "outside world")
- the test instantiated a separate wallet just because we need to have some BTC coming from somewhere. lnd is not getting the BTC directly from the coinbase rewards because the funds are not available immediately and I had issues with that: lnd typically doesn't expect to be the recipient of a coinbase rewards that has timelock restriction. (need to wait X blocks before the coinbase rewards is spendable)
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.
@nicolasburtey Renamed test from 02a-fund-bitcoind to 02a-fund-outside
it("funds default/outside bitcoind wallet", async () => {}
it("funds outside lnd node", async () => {}
- The funding tests now pass - But the checkIsBalanced starts failing after crediting lnd tests (TODO)
This reverts commit 0d34550.
@@ -62,38 +62,39 @@ it("createWallet", async () => { | |||
// expect(balance).toBe(0) | |||
}) | |||
|
|||
it("deposit to bitcoind", async () => { |
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 test is being commented out (just for now). Many of SpecterWallet methods consider the outside (default) wallet being bypassed in tests. In addition to also considering lnd as the onchain wallet.
}) | ||
|
||
// Now tests start to fail here (same reason: checkIsBalanced) |
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 is what I plan to continue working on... but @nicolasburtey let me know if this is the best path or if I should focus on something else.
But the rest of the tests don't
This reverts commit 3137c35.
Instead of just replacing the default client with the outside client, it seems appropriate to still keep a default client around for non- wallet bitcoin-node specific functionality
Dividing this into multiple tasks so that we can review and merge smaller branches, these being preparation for bitcoind fully replacing lnd for onchain transactions:
|
}) => { | ||
try { | ||
// TODO? how many transactions back? | ||
const transactions: [] = await bitcoindHotWalletClient.listTransactions(null, 1000) |
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.
1000 should be a separate const
incoming: false, | ||
}) | ||
// eslint-disable-next-line | ||
let fee |
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.
why are fee
and fee_
necessary?
WIP: Starting PR for #272