Feat/meta/sn#3991
Conversation
d26ab18 to
f9b5259
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3991 +/- ##
==========================================
- Coverage 27.79% 27.09% -0.71%
==========================================
Files 681 683 +2
Lines 46797 46280 -517
==========================================
- Hits 13006 12538 -468
- Misses 32553 32564 +11
+ Partials 1238 1178 -60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| func (m *Meta) blockHandler(ctx context.Context, buff <-chan *block.Header) { | ||
| for { | ||
| if len(buff) >= blockBuffSize-1 { |
There was a problem hiding this comment.
doubt it can be >
btw is it possible to do by channel writer(s). Would be more natural to me
There was a problem hiding this comment.
doubt it can be >
why? we read an item, do some magic that may take time, and then check again what size we have now, a new item may have come
btw is it possible to do by channel writer(s). Would be more natural to me
writer does not belong to this repo, it is sent to neo-go code
There was a problem hiding this comment.
and then check again
buff can initially have blockBuffSize-1 which is a capacity. This does not mean queue is full. Wouldnt be a big problem, but still
There was a problem hiding this comment.
but there is len, not cap. not sure i understand you
There was a problem hiding this comment.
I don't think this check is useful, you can't separate overflow from normally filled queue on the reader side, only writer can do this on attempt to write.
cthulhu-rider
left a comment
There was a problem hiding this comment.
finished. Lets fix everything and merge
@AnnaShaleva may u pls check chain configuration?
| // notification work; it is required to always read notifications without | ||
| // any blocking or making additional RPC. | ||
| notificationBuffSize = 100 | ||
| notificationBuffSize = 10000 |
There was a problem hiding this comment.
this is an address + pointer buff channel, what cons may we have? ~703KB of memory
There was a problem hiding this comment.
i can change it to any value. do you have a suggestion? i just had an experience with deadlock with neo-go subscription, so this is why this big value was chosen
There was a problem hiding this comment.
That's why I'm asking. Increasing buffer size is not a solution to the deadlock problem. It will make the problem more hidden, that's it.
If there's a deadlock, it should be fixed in some other (explicit) way.
There was a problem hiding this comment.
Increasing buffer size is not a solution to the deadlock problem
we are balancing b/w notification handling speed and storage speed. i guess it is another thing for exploring in load testing (yet not answered)
|
|
||
| // Height returns current side chaih block height. | ||
| func (s *SideChain) Height() uint32 { | ||
| return s.core.HeaderHeight() |
There was a problem hiding this comment.
Header height differs from block height. What exactly do you need here?
There was a problem hiding this comment.
Header height differs from block height
in which way? you mean a 1-block difference in values? i need it for async, but the same VUB calculation
There was a problem hiding this comment.
you mean a 1-block difference in values?
Not only. It can be large difference if, e.g. the node is in the process of syncing or stale. Headers synchronisation is a separate process, different from blocks synchronisation.
There was a problem hiding this comment.
the only thing i can suggest is to block any operation before the node is in sync
There was a problem hiding this comment.
The decision is up to you, I'm just warning. Either way, you should either adjust the documentation and method name or adjust the return value.
There was a problem hiding this comment.
From what I see it's used in meta signing only where header is more appropriate as "the last known current height network it operating with", suggest renaming the method or fixing the comment.
AnnaShaleva
left a comment
There was a problem hiding this comment.
Tests are failing. Other than that looks legit as a prototype.
| config.HFCockatrice.String(): 0, | ||
| config.HFDomovoi.String(): 0, | ||
| config.HFEchidna.String(): 0, | ||
| config.HFFaun.String(): 0, |
There was a problem hiding this comment.
Add a compatibility unit-test: check that hardforks starting from Aspid ending with config.HFLatestStable are enabled in the resulting chainCfg. On subsequent NeoGo update if latest stable hardfork is changed, you'll know about it.
| configneogo.HFCockatrice.String(): 0, | ||
| configneogo.HFDomovoi.String(): 0, | ||
| configneogo.HFEchidna.String(): 0, | ||
| configneogo.HFFaun.String(): 0, |
| @@ -0,0 +1,21 @@ | |||
| package util | |||
There was a problem hiding this comment.
util is always a bad name, think about a better one. CC @roman-khimov
There was a problem hiding this comment.
this is not the package i created, i just extended it. but agree, moved to another network package
They should be adopted, yes. And there are other problems. Config check and changelog are not accepted to the experimental things. |
cthulhu-rider
left a comment
There was a problem hiding this comment.
seems good to me overall
10_000_000 meant nothing, GAS factor is 1_0000_0000. Also, new value means something at least. Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
It is available starting from v0.119.0. This should speed broadcast up, which is critical for metadata chain. Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
Just drop the old one, use new instead, no functional changed in the new one. Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
| TimePerBlock: 50 * time.Millisecond, | ||
| }, | ||
| Magic: fsChainProtocol.Network + 1, | ||
| MemPoolSize: 0, |
|
|
||
| func (m *Meta) blockHandler(ctx context.Context, buff <-chan *block.Header) { | ||
| for { | ||
| if len(buff) >= blockBuffSize-1 { |
There was a problem hiding this comment.
I don't think this check is useful, you can't separate overflow from normally filled queue on the reader side, only writer can do this on attempt to write.
|
|
||
| func (m *Meta) notificationHandler(ctx context.Context, buff <-chan *state.ContainedNotificationEvent) { | ||
| for { | ||
| if len(buff) >= notificationBuffSize-1 { |
| on.m.Unlock() | ||
|
|
||
| if ok { | ||
| on.metaSvc.taskQueue <- storageTask{addr: addr, o: &sub.objHeader} |
There was a problem hiding this comment.
Does it make any difference? Local HEAD should be pretty fast.
| wg.Go(func() { m.storager(ctx, m.taskQueue) }) | ||
|
|
||
| m.chain.SubscribeForBlocks(m.bCh) | ||
| m.chain.SubscribeForNotifications(m.evsCh) |
There was a problem hiding this comment.
Seems like this can be done via JSON-RPC as well. Can we have this mode (non-local chain) for nodes with IR nearby?
|
|
||
| // PutObject forces [Meta] to index provided object. | ||
| func (m *Meta) PutObject(o *object.Object) error { | ||
| return m.metabase.Put(o) |
| if len(pubKeyBytes) != compressedECDSAPublicKeyLen { | ||
| panic(fmt.Sprintf("wrong public key len: expected %d, got %d", compressedECDSAPublicKeyLen, len(pubKeyBytes))) | ||
| } | ||
|
|
|
|
||
| // Height returns current side chaih block height. | ||
| func (s *SideChain) Height() uint32 { | ||
| return s.core.HeaderHeight() |
There was a problem hiding this comment.
From what I see it's used in meta signing only where header is more appropriate as "the last known current height network it operating with", suggest renaming the method or fixing the comment.
| })) | ||
| } | ||
|
|
||
| func metadataChainHardforks() map[string]uint32 { |
There was a problem hiding this comment.
Should follow FS chain HF config (heights can be different, but just enabling it like this can be problematic as well). At least for 0-height ones.
No description provided.