Skip to content
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

Default session expiration may be unexpected to a regular user #3509

Closed
carpawell opened this issue Jul 11, 2024 · 5 comments
Closed

Default session expiration may be unexpected to a regular user #3509

carpawell opened this issue Jul 11, 2024 · 5 comments
Labels
config Configuration format update or breaking change I3 Minimal impact S4 Routine U2 Seriously planned
Milestone

Comments

@carpawell
Copy link
Member

If session expiration is not specified, it is calculated. The way it is derived and the fact that it is not constant may confuse.

Current Behavior

I set 100ms block time and enabled sessions, however, did not specify session expiration. In the logs I saw

SessionExpirationTime is not set or wrong, setting default value

and thought that I was all right. However, it seems that the "default" is:

  1. not a constant (you may disagree, but i think it should be stable in any run, this is how i understand default value for a config)
  2. it does not make it if i specify smth shorter than a second:
    conf.SessionExpirationTime = int(protoCfg.TimePerBlock / time.Second)

Expected Behavior

The default is smth like 30 seconds and does not depend on a block time. At least missing expiration time with enabled expirations, should not ever lead to a 0-second expiration (unusable sessions in fact).

Possible Solution

  1. Make it const

or at least

  1. Do not allow 0 duration in any cases

Steps to Reproduce

Specify 100ms time block and try to use sessions.

Context

100ms time block time test run.

Regression

Not sure.

Your Environment

v0.106.2

@carpawell carpawell added bug Something isn't working U2 Seriously planned labels Jul 11, 2024
@AnnaShaleva AnnaShaleva added I3 Minimal impact config Configuration format update or breaking change S4 Routine and removed bug Something isn't working labels Jul 11, 2024
@AnnaShaleva AnnaShaleva added this to the v0.106.3 milestone Jul 11, 2024
@AnnaShaleva
Copy link
Member

In the logs I saw

There was also the value printed in the logs, that's the way how you can get a clue on the real value:

zap.Int("SessionExpirationTime", conf.SessionExpirationTime)
  1. Make it const

Won't be done, it should be dependent from block time. Log message should be adjusted to change "setting default value" to something more user-friendly.

  1. Do not allow 0 duration in any cases

Yes, this is a bug, should be fixed.

@carpawell
Copy link
Member Author

Won't be done, it should be dependent from block time.

And you made it not in blocks but in seconds, huh? I do not have a strong opinion here but a session is a network thing to me so it should rely on some expectations about how fast a caller can do what he wants (and I am not sure reading some info from chain is connected to blocks or their duration).

@roman-khimov
Copy link
Member

Sessions require snapshots, snapshots may affect block processing. 30s timeout with 15s is two blocks behind, but 300 blocks behind if you have 100ms blocks, a huge difference in terms of recency.

@carpawell
Copy link
Member Author

Sessions require snapshots, snapshots may affect block processing

Oh, I see, why isn't it (config value) in blocks then?

@roman-khimov
Copy link
Member

Mostly because that's the way C# node is configured. Users may also have some time-based preferences. But block-based default works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config Configuration format update or breaking change I3 Minimal impact S4 Routine U2 Seriously planned
Projects
None yet
Development

No branches or pull requests

3 participants