-
Notifications
You must be signed in to change notification settings - Fork 116
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
itest: add main universe server harness, turn on proof courier for all test cases #712
Conversation
1b4dbee
to
7519b84
Compare
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.
Looks good, useful cleanup and more dogfooding! Also great to see itest time drop by ~50 seconds on my machine.
The one bit I didn't see is where we modify the universe config for the tapd
instance we're using as a Universe, to accept issuance and transfer proofs? I though at least some of those four config flags are false by default.
That's a good point, needed to double check. But we do it for all instances we create here: taproot-assets/itest/tapd_harness.go Line 170 in aa31e22
|
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.
Smaller diff than I expected. Nice to get this done 👍
@@ -700,7 +700,8 @@ func (p *ChainPorter) transferReceiverProof(pkg *sendPackage) error { | |||
|
|||
// If we have a proof courier instance active, then we'll launch several | |||
// goroutines to deliver the proof(s) to the receiver(s). | |||
if p.cfg.ProofCourierCfg != nil { | |||
_, isPreSigned := pkg.Parcel.(*PreSignedParcel) | |||
if p.cfg.ProofCourierCfg != nil && !isPreSigned { |
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.
I think you should include the commit message body as a code comment here to explain why the pre-signed status matters at this point.
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.
Great point, added.
Since a pre-signed parcel (a parcel that uses the RPC driven vPSBT flow) doesn't have proof courier URLs (they aren't part of the vPSBT), the proofs must always be delivered in an interactive manner from sender to receiver and we don't even need to attempt to use a proof courier.
With this commit we remove the use of the proof.DisabledCourier courier by default and instead use the universe RPC proof courier if nothing is specified in the integration test case list.
The original server harness was meant to be the actual universe RPC server from the beginning. So we can merge the two into a single one and start/use that by default.
With this commit we remove all instances of the manual proof transfer from call sites that do an actual non-interactive (address based) transfer, for which a proof courier should now handle the proof transmission.
By default we want all new tapd instances to be synced with the main universe server, so we add it as a federation member unless specifically disabled.
With this commit we optimize the backoff config to enable much faster proof transmission during integration tests. A significant speedup also comes from reducing the time the custodian waits between detecting a transfer on-chain and attempting to fetch the proof with the courier.
Since we now have a main universe server by default, we can disable test nodes syncing directly to each other by default.
7519b84
to
6361160
Compare
Fixes #649.
This PR turns on the universe RPC proof courier by default for all integration tests that previously didn't specify a proof courier.
While doing that, we also remove some duplication with the proof courier and the universe server harness, which then allows us to do a bit of more cleanup.