Custom channel testing bugfixes #1011
Conversation
Fixes #1007 by using a higher value for testnet, if the user doesn't supply their own custom number.
The variable name was incorrect in the first place, so we fix that to properly represent how the mock price is currently interpreted. And to allow for different ways of specifying a mock price, we move the static calculation to the constructor.
guggero
force-pushed
the
tap-channel-testing-bugfixes
branch
2 times, most recently
from
2d8996a to
924656b
Compare
sample-tapd.conf
Outdated
| ; Mock price oracle static asset units per BTC rate (for example number of USD | ||
| ; cents per BTC if one asset unit represents a USD cent); use either this or | ||
| ; mockoraclesatsperasset | ||
| ; experimental.rfq.mockoracleassetsperbtc= | ||
|
|
||
| ; Mock price oracle static satoshis per asset unit rate (for example number of | ||
| ; satoshis to pay for one USD cent if one asset unit represents a USD cent); use | ||
| ; either this or mockoracleassetsperbtc | ||
| ; experimental.rfq.mockoraclesatsperasset= |
There was a problem hiding this comment.
I think we should mention here that we're looking for a uint and that's why a user might choose one parameter over another. I think we need to make that clear in the CLI args also.
Another way of thinking about that is: what happens if a user attempts to use a float with either of these arguments? Will they see an error message?
There was a problem hiding this comment.
Good point. Added some additional text.
If a user specifies a fraction, they'll get this error: could not load config: error parsing flags: invalid argument for flag --taproot-assets.experimental.rfq.mockoracleassetsperbtc' (expected uint64): strconv.ParseUint: parsing "12.3": invalid syntax`
| if numAssetUnits == 0 { | ||
| return 0, nil, fmt.Errorf("asset unit price (%d mSat per "+ | ||
| "asset unit) too high to represent HTLC value of %v", | ||
| mSatPerAssetUnit, totalAmount) | ||
| } |
There was a problem hiding this comment.
If the price of the asset is very high (in sats), then shouldn't we just set numAssetUnits to 1 here (if it were previously calculated to be 0)?
From what I can tell, what we're trying to do at this point and in that situation, is to spend an expensive asset to settle an invoice. And if we return an error on numAssetUnits == 0 we're saying: "the smallest unit of this asset is more expensive than the invoice you're trying to settle, so we wont let you spend it". But perhaps the user still wants to spend that asset unit in that way, even if they're overpaying.
I guess it's fine to return an error as you've added here. And we can always add an "accept overpay" flag or something later.
There was a problem hiding this comment.
Hmm, I don't think we should just silently overpay, as that'll definitely be exploited at some point. I really think this falls in the same precision category as with the MPP rounding error, which should be fixed by minting assets with a sufficient number of units per BTC.
To make the use of the mock oracle price flag way more clear, we first rename it to correctly represent its usage. We then also make sure a minimum value is set, otherwise one asset unit would just cost too much and cause issues when trying to send them over the network. To make it easier to express what one unit should cost, we also add a second price flag that can specify the number of satoshis per asset unit, which will make it easier for users that want to represent something that doesn't have an official exchange rate.
Fixes #1006. In this commit we make sure that we'll never end up attempting to send a zero asset amount HTLC, which would then fail further down the line. A zero amount can happen if the resolution of asset unit to satoshis is insufficient and one unit costs more than the satoshi amount that should be represented in the HTLC. The main mitigation for this is on the issuer side that they make sure the decimal display value is high enough and one asset unit represents a fraction of a cent (and is therefore close to being equal to one satoshi).
Fixes #1010. With this commit we introduce a new fn.CriticalError type and only send RFQ errors to the main error channel (which causes the daemon to shut down) if an error is critical to the operation and can't just be logged.
To be able to have some stateless server components be ready before the server is fully started up, we do need to have the chain params available. So we add them separately from the main config.
This fixes a startup dependency issue between lnd and tapd where both wait for each other to become ready. Because the aux leaf creator doesn't depend on any other components that need to be started first, and also doesn't carry any state by itself, we can declare it to be fully stateless and not block on server startup before being able to use it.
We allow any stateless methods of the aux leaf signer to be called without the server needing to be fully started, so reduce the number of potential startup wait dependencies there could be.
guggero
force-pushed
the
tap-channel-testing-bugfixes
branch
from
924656b to
01be6ee
Compare
| } | ||
|
|
||
| // NewMockPriceOracle creates a new mock price oracle. | ||
| func NewMockPriceOracle(expiryDelay, usdPerBTC uint64) *MockPriceOracle { | ||
| func NewMockPriceOracle(expiryDelay, assetsPerBTC uint64) *MockPriceOracle { | ||
| mSatPerAsset := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) / |
There was a problem hiding this comment.
Related, how does this internal mSatPerAsset get updated (if at all?)
Non-blocking.
There was a problem hiding this comment.
It's never updated in the mock. It's a static value configured in the config or CLI args.
|
|
||
| // NewAuxLeafCreator creates a new Taproot Asset auxiliary leaf creator based on | ||
| // the passed config. | ||
| func NewAuxLeafCreator(cfg *LeafCreatorConfig) *AuxLeafCreator { |
There was a problem hiding this comment.
FWIW, I think we could've kept the struct structure for ease of use/isolation, in say tests.
Fixes #1007.
Fixes #1006.
Fixes #1010.
Also adds a fix for the startup dependency issue between
lndandtapd: Apparently it's possible thatlndwants to force close channels before the RPC server is fully started. That then calls into the aux leaf creator which blocks untiltapdthinks it's fully started (which in turn depends on a successful connection tolnd's RPC).By making the aux leaf creator fully stateless and allowing some methods of the aux leaf signer that are stateless to be called early, we should be able to fix this startup dependency issue.