Advance bootstrap when any network ready

[alkum]

Advance through the initialization process as soon as any network is bootstrapped.

[chimp1984]

Shouldn't it be result==true?

[alkum]

Comment says "If networkNode has not been created..." so my understanding is in that case result is false?

[chimp1984]

Hm... dont remember exaclty. Its a bit unclear and should be improved. I think it was meaning that if the network was not initialized already (I guess we call it just in case and after once initialized it returns fast) we need to persist the keypair. Best to put a breakpoint in to see what the value of result is and if its called repeatedly...

[chimp1984]

The bootstrapToNetwork method below seems to return always true if successful.

[alkum]

This specific line (if (result == false)) is called only once, even when using all 3 networks, with result being true. So the if condition is never true.

If I change it to if (result == true), persisting the network in the very next line fails with

NetworkId for default must be present

This makes sense, because bootstrapping != initializing the network. The network ID is only available after the networkService is initialized, but here we only do bootstrapping.

NetworkService.initialize is relatively long, because it only finishes when all enabled networks are up and running.

Would it make sense then to remove the whole thenApply block?

return serviceNodesByTransport.bootstrapToNetwork(getDefaultPortByTransport(), nodeId)
        .thenApply(result -> { // We use thenApply to return a future that completes after this function
            // If networkNode has not been created we create now one with the default pubKey (default keyId)
            // and persist it.
            if (result == false) {
                persistNetworkId(nodeId, keyPairService.getDefaultPubKey());
            }
            return result;
        });

Because its not executed (boolean check is called only once with result = true). And persisting the network would fail because right after bootstrapping the network is too early (it would work after network initialize).

[chimp1984]

I checked out your PR and looked into that. The if (result == false) check is wrong as we always return true from serviceNodesByTransport.bootstrapToNetwork. That API is questionable in itself and would require more refactoring work...

[chimp1984]

Currently the addresses are created when the user clicks next so the time being on that screen does not help us as the user can change the keypair any time, thus we cannot create the addresses upfront. But we should consider to bake something into the UI which allows us to do that in the background. Maybe we dont need the popup but delay it to the time when the address is actually needed (at create offer later, until then the address shoudld be created, and only if not we need to show the user a "in-progress" screen

[alkum]

Currently the addresses are created when the user clicks next

Correct, but the address creation is relatively fast compared to network init. Also, address creation waits on complete network initialization (e.g. clearnet + tor + i2p).

...so the time being on that screen does not help us as the user can change the keypair any time.

In my tests (clearnet + tor) it helped. It didn't eliminate the need to show the popup because as you said address creation takes a bit of time. But it took way less time if some time was spent on that screen vs if user tried to click next right away.

I spent some time trying to optimize it (make address creation depend on network bootstrap, which advances after first network is ready, vs network init, which completes when all networks are ready). But I couldn't, because the addresses indeed need an initialized network:

bisq2/identity/src/main/java/bisq/identity/IdentityService.java

Lines 156 to 160 in 4f1cf90

	public CompletableFuture<Identity> createNewInitializedIdentity(String profileId, String keyId, KeyPair keyPair) {
	keyPairService.persistKeyPair(keyId, keyPair);
	PubKey pubKey = new PubKey(keyPair.getPublic(), keyId);
	String nodeId = StringUtils.createUid();
	return networkService.getInitializedNetworkId(nodeId, pubKey)

So I added that comment to spare future efforts spent in this direction.

Maybe we dont need the popup but delay it to the time when the address is actually needed (at create offer later, until then the address shoudld be created, and only if not we need to show the user a "in-progress" screen

Good idea. I'll prepare a PR to do that.

[chimp1984]

Yes, I think best is to remvoe that popup and check if network addresses have been created once they are needed. The identity creating needs its so we need to keep a temporary state until we know all addresses.

[alkum]

Should I do that in this PR or a separate one?

[chimp1984]

Could you rebase that PR.

[alkum]

Rebased, but there are still 2 open points in the review.

[chimp1984]

I think that PR is too old now to be easy to get merged. Will close it for now.