node: Break notary deposit wait after VUB #1467

dstepanov-yadro · 2024-10-31T10:15:00Z

dstepanov-yadro commented

2024-10-31 10:15:00 +00:00

There is no point in waiting for the transaction to complete after VUB.

dstepanov-yadro force-pushed fix/deposit_vub from 6fced12a69 to 1facf94b49

2024-10-31 10:15:42 +00:00

Compare

dstepanov-yadro requested review from storage-core-committers 2024-10-31 10:21:52 +00:00

dstepanov-yadro requested review from storage-core-developers 2024-10-31 10:21:53 +00:00

acid-ant approved these changes 2024-10-31 10:27:39 +00:00

Dismissed

fyrchik reviewed 2024-10-31 11:27:15 +00:00

cmd/frostfs-node/morph.go Outdated

					
				@ -192,2 +192,4 @@

							return fmt.Errorf("could not wait for one block in chain: %w", err)

						}

						currHeight, err := c.cfgMorph.client.BlockCount()

fyrchik commented

2024-10-31 11:27:15 +00:00

This is tricky, because there is some time between TxHalt() and BlockCount()
Consider this scenario:

Check for TxHalt() at Height=VUB-1
New block is accepted, tx is in it.
Wait for one more block
Height = VUB + 1, so we exit.

I think we must make yet another TxHalt request in case currHeight > vub.

Still we may switch endpoint, but this is a more rare event.
Another option would be to make WaitTxPersist(ctx, tx) and do everything under switch mutex.

This is tricky, because there is some time between `TxHalt()` and `BlockCount()` Consider this scenario: 1. Check for `TxHalt()` at Height=VUB-1 2. New block is accepted, tx is in it. 3. Wait for _one more_ block 4. Height = VUB + 1, so we exit. I think we must make yet another `TxHalt` request in case `currHeight > vub`. Still we may switch endpoint, but this is a more rare event. Another option would be to make `WaitTxPersist(ctx, tx)` and do everything under switch mutex.

👍 1

fyrchik commented

2024-10-31 11:42:14 +00:00

Also, can we reuse something from https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/waiter/waiter.go ?

dstepanov-yadro commented

2024-10-31 12:42:36 +00:00

Done: make yet another TxHalt request in case currHeight > vub

Done: make yet another `TxHalt` request in case `currHeight > vub`

dstepanov-yadro commented

2024-10-31 12:44:34 +00:00

Also, can we reuse something from https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/waiter/waiter.go ?

Don't think we need one more dependency from neo-go.

> Also, can we reuse something from https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/waiter/waiter.go ? Don't think we need one more dependency from neo-go.

fyrchik commented

2024-10-31 12:55:26 +00:00

We already depend on the module (and it is external).
And that package canonically solves exactly our problem (I have given 1 error, who knows what else can go wrong).

We already depend on the module (and it is external). And that package canonically solves exactly our problem (I have given 1 error, who knows what else can go wrong).

dstepanov-yadro commented

2024-10-31 13:28:08 +00:00

Then why do you ask, if in fact you insist on using it?

dstepanov-yadro commented

2024-10-31 13:55:49 +00:00

fixed

fyrchik commented

2024-10-31 14:18:08 +00:00

That seemed like an obvious choice, and I wanted to know if there was any reason not to make it.
I find yet-another-dependency (which in fact is already is in go.mod) argument weak.

That seemed like an obvious choice, and I wanted to know if there was any reason not to make it. I find yet-another-dependency (which in fact is already is in `go.mod`) argument weak.

dstepanov-yadro commented

2024-10-31 14:52:27 +00:00

obvious choice

I don't see an obvious choice: to use this interface, which I didn't even know about (and not only me), I had to add a structure and two methods, and also add a dependency on a package that can be changed at any time (do you know how neo-go supports backward compatibility?)
The implementation itself also raises questions:

the context should not be stored in the structure:

RPCPollingBased interface {
		Context() context.Context

there is no check for the length of the array Execution: res.Executions[0]
the number of retries const PollingBasedRetryCount = 3 is not configured
why are retries used only to request the number of blocks, but not for application log
...

> obvious choice I don't see an obvious choice: to use this interface, which I didn't even know about (and not only me), I had to add a structure and two methods, and also add a dependency on a package that can be changed at any time (do you know how neo-go supports backward compatibility?) The implementation itself also raises questions: - the context should not be stored in the structure: ``` RPCPollingBased interface { Context() context.Context ``` - there is no check for the length of the array `Execution: res.Executions[0]` - the number of retries `const PollingBasedRetryCount = 3` is not configured - why are retries used only to request the number of blocks, but not for application log - ...

dstepanov-yadro force-pushed fix/deposit_vub from 1facf94b49 to ce13b46281

2024-10-31 12:41:18 +00:00

Compare

dstepanov-yadro dismissed acid-ant's review 2024-10-31 12:41:18 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dstepanov-yadro force-pushed fix/deposit_vub from ce13b46281 to 2bcdb65378

2024-10-31 13:52:40 +00:00

Compare

dstepanov-yadro force-pushed fix/deposit_vub from 2bcdb65378 to 99240923a2

2024-10-31 13:53:00 +00:00

Compare

dstepanov-yadro force-pushed fix/deposit_vub from 99240923a2 to 1fd704a376

2024-10-31 14:10:37 +00:00

Compare

fyrchik commented

2024-10-31 14:18:58 +00:00

on a side note: systemd status will update after we have made bootstrap request, right?

dstepanov-yadro commented

2024-10-31 14:26:15 +00:00

on a side note: systemd status will update after we have made bootstrap request, right?

yes

> on a side note: systemd status will update after we have made bootstrap request, right? yes

fyrchik reviewed 2024-11-01 11:04:05 +00:00

cmd/frostfs-node/morph.go Outdated

					
				@ -197,0 +201,4 @@

						}

						return fmt.Errorf("could not wait for notary deposit persists in chain: %w", err)

					}

					if success := res.Execution.VMState.HasFlag(vmstate.Halt); success {