-
Notifications
You must be signed in to change notification settings - Fork 81
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
Headers fetching via NeoFS BlockFetcher service #3789
Conversation
ee083c5
to
6b9d29e
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.
HeadersFetcher is started in the wrong place. Ensure that nothing prevents StateSync module to request blocks/headers during the node start.
pkg/network/server.go
Outdated
var err error | ||
s.blockFetcher, err = blockfetcher.New(chain, s.NeoFSBlockFetcherCfg, log, s.bFetcherQueue.PutBlock, | ||
sync.OnceFunc(func() { close(s.blockFetcherFin) })) | ||
s.headerFetcher, err = blockfetcher.New(chain, s.NeoFSBlockFetcherCfg, log, s.bFetcherQueue.PutBlock, s.bFetcherQueue.PutHeader, |
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.
Provide s.stateSync
instead of chain
as the first argument to blockfetcher.New
.
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.
The height is different for s.stateSync. It starts from 3 mln, so the block/header can persist, and the queue becomes locked. That's why I rolled it back to the chain. Why can't it be the chain? Blockfetcher uses it only for config and BlockHeight(). will recheck why its s.stateSync starts from the middle of the sequence.
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.
fixed
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.
We need BlockHeight and HeaderHeight to be aligned with statesync module vision, that's the main idea.
e867abb
to
26c546e
Compare
26c546e
to
3b2b9a6
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.
Does the service start work as expected?
pkg/core/blockchain.go
Outdated
@@ -285,9 +285,6 @@ func NewBlockchain(s storage.Store, cfg config.Blockchain, log *zap.Logger) (*Bl | |||
zap.Uint32("MaxValidUntilBlockIncrement", cfg.MaxValidUntilBlockIncrement)) | |||
} | |||
if cfg.P2PStateExchangeExtensions { | |||
if !cfg.StateRootInHeader { | |||
return nil, errors.New("P2PStatesExchangeExtensions are enabled, but StateRootInHeader is off") | |||
} |
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.
We need more sophisticated check. If BlockFetcher is on, then it's OK, but if Blockfetcher is off and StateRootInHeader is also off, then it's an error.
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.
Fix error message.
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.
Now, when we have NeoFSStateSyncExtensions
this conversation is not relevant. Untie NeoFSStateSyncExtensions
from P2PStateExchangeExtensions
, these are two different settings. Implement esparate verification logic for NeoFSStateSyncExtensions
.
pkg/network/server.go
Outdated
if s.chain.HeaderHeight() < p.LastBlockIndex() { | ||
return s.requestHeaders(p) | ||
} |
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.
This if
check must be adjusted. Right now it's related to peers only, but if blockfetcher is used, then we don't care about peer height because another source of data is used (NeoFS storage).
pkg/network/server.go
Outdated
if s.stateSync.NeedHeaders() { | ||
if s.chain.HeaderHeight() < p.LastBlockIndex() { | ||
return s.requestHeaders(p) | ||
} | ||
return nil | ||
} | ||
s.headerFetcher.Shutdown() | ||
if s.ServerConfig.NeoFSBlockFetcherCfg.Enabled { | ||
err := s.blockFetcher.Start() |
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.
Blockfetcher should fetch exactly the range of blocks that is required by statesymc module if statesync module is enabled. If statesync module is disabled, then blockfetcher should fetch all blocks starting from 0. Check that this logic is present in the blockfetcher.
pkg/network/server.go
Outdated
if s.ServerConfig.NeoFSBlockFetcherCfg.Enabled { | ||
err := s.headerFetcher.Start() | ||
if err != nil { | ||
s.log.Error("skipping NeoFS BlockFetcher", zap.Error(err)) | ||
} | ||
} | ||
if s.headerFetcher.IsActive() { | ||
return nil | ||
} |
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.
This condition may lead to deadlock, consider the situation: NeoFS storage is missing the latest headers. Once started, headerfetcher will fetch all available headers and shutdown. However, if some latest headers are still required by statesync, then we need to switch to P2P. Right now this code will restart headersfetcher anyway. Setting s.headerFetcher
to nil after the first headerfetcher shutdown (and checking for nil in dependent places) may do the trick.
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.
with nil we cant use isActive.
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.
Without nil we can't distinguish finished service from not started. So additional nil checks is a trade-off.
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 have added isShutdown to the blockfetcher. if you don't like it, will look more precisely to the nil.
err = bfs.enqueueBlock(b) | ||
if !bfs.cfg.BlocksOnly { | ||
err = bfs.enqueueHeader(&b.Header) |
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.
We need the code that will read header from stream without transactions reading.
pkg/network/server.go
Outdated
s.bFetcherQueue = bqueue.New(chain, log, nil, s.NeoFSBlockFetcherCfg.BQueueSize, updateBlockQueueLenMetric, bqueue.Blocking) | ||
s.bFetcherQueue = bqueue.New(s.stateSync, log, nil, s.NeoFSBlockFetcherCfg.BQueueSize, updateBlockQueueLenMetric, bqueue.Blocking) |
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.
We may need a switch here. The main idea is that blockfetcher should work both with statesync module enabled/disabled. If statesync module is disabled then we need a usual blockfetcher operation flow that will fetch blocks, put them directly into chain-backed queue and shutdown. If statesync module is enabled then blockfetcher should use statesync-backed queue and headersfetcher should also be started.
3b2b9a6
to
f4d006f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3789 +/- ##
==========================================
- Coverage 83.02% 82.82% -0.20%
==========================================
Files 336 337 +1
Lines 47090 47351 +261
==========================================
+ Hits 39095 39218 +123
- Misses 6395 6524 +129
- Partials 1600 1609 +9 ☔ View full report in Codecov by Sentry. |
f4d006f
to
b2c9187
Compare
sometimes:
|
2112fe0
to
f3629c4
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.
Better, but still needs polishing.
pkg/core/blockchain.go
Outdated
@@ -285,9 +285,6 @@ func NewBlockchain(s storage.Store, cfg config.Blockchain, log *zap.Logger) (*Bl | |||
zap.Uint32("MaxValidUntilBlockIncrement", cfg.MaxValidUntilBlockIncrement)) | |||
} | |||
if cfg.P2PStateExchangeExtensions { | |||
if !cfg.StateRootInHeader { | |||
return nil, errors.New("P2PStatesExchangeExtensions are enabled, but StateRootInHeader is off") | |||
} |
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.
Fix error message.
pkg/core/statesync/module.go
Outdated
@@ -131,6 +136,9 @@ func (s *Module) Init(currChainHeight uint32) error { | |||
if p < 2*s.syncInterval { | |||
// chain is too low to start state exchange process, use the standard sync mechanism | |||
s.syncStage = inactive | |||
if s.syncStage != oldStage { | |||
s.notifyStageChanged() | |||
} |
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.
Move it to the defered statement.
pkg/core/statesync/module.go
Outdated
|
||
// stageCallback is an optional callback that is triggered whenever | ||
// the sync stage changes. | ||
stageCallback func() |
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.
stageChangedCallback?
pkg/core/statesync/module.go
Outdated
// notifyStageChanged triggers stage callback if it's set. | ||
func (s *Module) notifyStageChanged() { | ||
if s.stageCallback != nil { | ||
go s.stageCallback() |
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.
NACK, not a proper place for goroutine, make it synchronious on the module side.
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.
is it fixed in the current implementation? moved it to go notifyStageChanged()
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.
No.
pkg/core/statesync/module.go
Outdated
if s.syncStage != oldStage { | ||
s.notifyStageChanged() | ||
oldStage = s.syncStage | ||
} |
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.
defineSyncStage
is an atomic operation, you can't notify in the middle of it.
@@ -140,6 +143,18 @@ func New(chain Ledger, cfg config.NeoFSBlockFetcher, logger *zap.Logger, putBloc | |||
if err != nil { | |||
return nil, err | |||
} | |||
|
|||
getCfg := chain.GetConfig() | |||
m := smartcontract.GetDefaultHonestNodeCount(int(getCfg.ValidatorsCount)) |
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.
You need to make it adaptive. Validators count depends on the node's setting, we have pre-defined list of heights where validators count may be updated. See CommitteeUpdateHistory.
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.
not yet implemented.
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.
As we do Get on oid we do not know the index, so we can't check the relevant for this index Validation count.
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.
we can do something like this: pass to the oidsCh oid and Index, but its not really cool
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.
pass to the oidsCh oid and Index
Do it. Implement GETRANGE-based solution for index-files-based headers search and for single-object header search, keep the old solution for SEARCH-based headers search (just set the index to -1 to reuse the same oidsCh
). Create an issue for the latter case.
if isContextCanceledErr(err) { | ||
return | ||
} | ||
bfs.log.Error("failed to decode block from stream", zap.String("oid", blkOid.String()), zap.Error(err)) |
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.
Adjust error text, it's not block anymore.
495af54
to
49fe75d
Compare
pkg/core/statesync/module.go
Outdated
@@ -509,7 +568,7 @@ func (s *Module) GetUnknownMPTNodesBatch(limit int) []util.Uint256 { | |||
return s.mptpool.GetBatch(limit) | |||
} | |||
|
|||
// HeaderHeight returns the height of the latest header. |
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.
rollback
pkg/network/bqueue/queue.go
Outdated
// New creates an instance of BlockQueue. | ||
func New(bc Blockqueuer, log *zap.Logger, relayer func(*block.Block), cacheSize int, lenMetricsUpdater func(l int), mode OperationMode) *Queue { | ||
// New creates an instance of Queue that handles both blocks and headers. | ||
func New(bc Blockqueuer, log *zap.Logger, relayer any, cacheSize int, lenMetricsUpdater func(l int), mode OperationMode, objectMode TypeMode) *Queue { |
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.
relayer
is not any
, it's a function with pre-defined signature.
pkg/network/bqueue/queue.go
Outdated
func (bl *Queue) add(item Queueble) error { | ||
if b, ok := item.(*block.Block); ok { | ||
return bl.chain.AddBlock(b) | ||
} | ||
if h, ok := item.(*block.Header); ok { | ||
return bl.chain.AddHeaders(h) | ||
} | ||
return nil | ||
} |
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.
NACK. I literally mean make the queue generic:
var bQueue := New[Block](blockAdapter{chain}, ...)
var bSyncQueue := New[Block](blockAdapter{stateSyncModule}, ...)
var hSyncQueue := New[Header](headerAdapter{stateSyncModule}, ...)
With func New[Q Queueable](bc Blockqueuer[Q], ...)
. Then implement Blockqueuer
over blockAdapter
and headerAdapter
. And then inside the queue use bq.chain.AddItem(item)
for a single block/header or bq.chain.AddItems(item)
for a multiple headers, that's it.
You will need to modify Blockqueuer:
type Blockqueuer[Q Queueable] interface {
AddItem(Q) error
AddItems(...Q) error
Height() uint32
}
pkg/core/block/block.go
Outdated
// GetIndex returns the index of the block. | ||
func (b *Block) GetIndex() uint32 { | ||
return b.Header.Index | ||
} | ||
|
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.
You don't need that, Header
already has this definition.
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.
Not fixed.
pkg/core/statesync/module.go
Outdated
s.stageChangedCallback = cb | ||
} | ||
|
||
// notifyStageChanged triggers stage callback if it's set. |
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.
It's not "stage callback". It notifies about state changes.
pkg/core/statesync/module.go
Outdated
@@ -351,6 +399,9 @@ func (s *Module) AddBlock(block *block.Block) error { | |||
s.syncStage |= blocksSynced | |||
s.log.Info("blocks are in sync", | |||
zap.Uint32("blockHeight", s.blockHeight)) | |||
if s.syncStage != oldStage { | |||
s.notifyStageChanged() | |||
} |
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.
What I mean is that you need to move notification to the checkSyncIsCompleted
and pass the old stage as an argument.
pkg/core/statesync/module.go
Outdated
@@ -351,6 +399,9 @@ func (s *Module) AddBlock(block *block.Block) error { | |||
s.syncStage |= blocksSynced | |||
s.log.Info("blocks are in sync", | |||
zap.Uint32("blockHeight", s.blockHeight)) | |||
if s.syncStage != oldStage { | |||
s.notifyStageChanged() | |||
} |
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.
But it actually won't work properly because we need to notify only after mutex is removed. Hence, just move this to the defered statement to the start of the function.
pkg/network/server.go
Outdated
@@ -1136,7 +1209,7 @@ func (s *Server) handleGetHeadersCmd(p Peer, gh *payload.GetBlockByIndex) error | |||
|
|||
// handleHeadersCmd processes headers payload. | |||
func (s *Server) handleHeadersCmd(p Peer, h *payload.Headers) error { | |||
if s.blockFetcher.IsActive() { | |||
if s.syncHeaderFetcher.IsActive() { |
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.
Not fixed.
pkg/core/transaction/witness.go
Outdated
@@ -16,6 +16,9 @@ const ( | |||
// MaxVerificationScript is the maximum allowed length of verification | |||
// script. It should be appropriate for 11/21 multisignature committee. | |||
MaxVerificationScript = 1024 | |||
|
|||
// DefaultInvocationScriptSize is the default length of invocation script with one signature. | |||
DefaultInvocationScriptSize = 66 |
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.
Line width. And it's relevant not only for Transaction, think about better place for it. Comment must be more specific, it's not about "default", tell something about Crypto.System.CheckSig. VM has definition of this constant, may be we can export.
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.
} | ||
|
||
// New creates a new BlockFetcher Service. | ||
func New(chain Ledger, cfg config.NeoFSBlockFetcher, logger *zap.Logger, putBlock func(*block.Block) error, shutdownCallback func()) (*Service, error) { | ||
func New(chain Ledger, cfg config.NeoFSBlockFetcher, logger *zap.Logger, put func(any) error, shutdownCallback func(), typeMode bqueue.TypeMode) (*Service, error) { |
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.
You need to define this mode for blockfetcher, not for queue.
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.
And why typeMode
when it's an operationMode
?
231f7d7
to
c3af507
Compare
pkg/network/server.go
Outdated
@@ -59,7 +59,7 @@ type ( | |||
Ledger interface { | |||
extpool.Ledger | |||
mempool.Feer | |||
bqueue.Blockqueuer | |||
bqueue.Blockqueuer[bqueue.Queueable] |
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.
You need new interface for that. Starting from this commit bqueue.Blockqueuer
serves only queue needs.
pkg/network/server.go
Outdated
|
||
s.bSyncQueue = bqueue.New(s.stateSync, log, nil, bqueue.DefaultCacheSize, updateBlockQueueLenMetric, bqueue.NonBlocking) | ||
s.bSyncQueue = bqueue.New[*block.Block](blockAdapter{s.stateSync}, log, nil, bqueue.DefaultCacheSize, updateBlockQueueLenMetric, bqueue.NonBlocking) |
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.
You will have two adapters for statesync module. So it's more like stateSyncBlockQueueAdapter
and stateSyncHeaderQueueAdapter
for statesync module and chainBlockQueueAdapter
.
pkg/network/server.go
Outdated
@@ -282,6 +279,48 @@ func newServerFromConstructors(config ServerConfig, chain Ledger, stSync StateSy | |||
return s, nil | |||
} | |||
|
|||
type blockAdapter struct { | |||
bc StateSync |
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.
It's not bc
here, it's stateSync
.
pkg/network/server.go
Outdated
@@ -282,6 +279,48 @@ func newServerFromConstructors(config ServerConfig, chain Ledger, stSync StateSy | |||
return s, nil | |||
} | |||
|
|||
type blockAdapter struct { |
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.
Move all adapters to a separate file. Declare a variable to ensure it implements proper interface, like `var _ = (queue.Blockqueuer[*block.Block])(&blockAdapter{})
pkg/network/server.go
Outdated
} | ||
|
||
func (a blockAdapter) AddItem(b *block.Block) error { | ||
return a.bc.AddItem(b) |
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.
Statesync module itself must not implement any Item-related things. That's exactly the purpose of adapters. Use AddBlock method here.
pkg/network/server.go
Outdated
func (a blockAdapter) AddItems(bs ...*block.Block) error { | ||
for _, b := range bs { | ||
if err := a.bc.AddItem(b); err != nil { | ||
return err | ||
} | ||
} | ||
return nil |
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.
This must not be supported for blocks, it's only designated for headers. Panic.
pkg/network/server.go
Outdated
} | ||
|
||
func (a chainAdapter) AddItem(b *block.Block) error { | ||
return a.chain.AddItem(b) |
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.
Ditto for the blockchain, no AddItem
is defined on the blockchain.
pkg/network/server.go
Outdated
} | ||
|
||
func (a chainAdapter) Height() uint32 { | ||
return a.chain.Height() |
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.
Ditto for height. It must be BlockHeight() for blocks adapter and HeaderHeight() for headers adapter.
pkg/network/server.go
Outdated
for _, header := range h.Hdrs { | ||
err := s.stateSync.AddItems(header) |
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.
You must use AddItems(h.Hrs...)
, otherwise it's useless. This operation must be batched.
pkg/network/state_sync.go
Outdated
@@ -9,12 +10,15 @@ import ( | |||
// StateSync represents state sync module. | |||
type StateSync interface { | |||
AddMPTNodes([][]byte) error | |||
bqueue.Blockqueuer | |||
bqueue.Blockqueuer[bqueue.Queueable] |
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.
Ditto for statesync, bqueue.Blockqueuer[bqueue.Queueable]
is now queue-related. Create one more interface with specific methods and reuse it for statesync and for Ledger.
c3af507
to
4a9c2eb
Compare
4a9c2eb
to
646625b
Compare
|
||
for height := range chain.CommitteeHistory { | ||
m = smartcontract.GetDefaultHonestNodeCount(chain.GetNumOfCNs(height)) | ||
verification, _ = smartcontract.CreateDefaultMultiSigRedeemScript(vs[:chain.GetNumOfCNs(height)]) |
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.
should it be new vs
here? in CommitteeHistory
we do not have public keys. or it should be ValidatorsHistory?
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.
You don't really care about particular keys, size estimators doesn't need it.
} | ||
index++ //Won't work properly if neofs.ObjectSearch results are not ordered. |
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.
full get for this mode
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.
or headers as an object(s) in the container
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.
or adapt batchSize to headerSizeMap
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.
Process queue feedback first, then update PR and I'll review the rest, there's a lot of things that need to be fixed.
internal/fakechain/fakechain.go
Outdated
@@ -49,6 +49,10 @@ type FakeStateSync struct { | |||
AddMPTNodesFunc func(nodes [][]byte) error | |||
} | |||
|
|||
func (s *FakeStateSync) HeaderHeight() uint32 { | |||
return s.HeaderHeight() |
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.
What exactly is s.HeaderHeight()
in this context? You're implementing interface, you can't use methods of this interface in the implementation, it just won't work.
pkg/consensus/consensus.go
Outdated
@@ -58,7 +59,7 @@ type Ledger interface { | |||
|
|||
// BlockQueuer is an interface to the block queue manager sufficient for Service. | |||
type BlockQueuer interface { | |||
PutBlock(block *coreb.Block) error | |||
Put(block bqueue.Queueable) error |
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.
It's not block
anymore. It's something queueable.
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.
*coreb.Block can be saved?
pkg/core/block/block.go
Outdated
// GetIndex returns the index of the block. | ||
func (b *Block) GetIndex() uint32 { | ||
return b.Header.Index | ||
} | ||
|
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.
Not fixed.
dd6c338
to
0997a77
Compare
0997a77
to
18c8d20
Compare
pkg/network/bqueue/queue.go
Outdated
} | ||
|
||
// Queue is the queue of queueable elements. | ||
type Queue[Q queueable] struct { |
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.
You can't use non-exported type for an exported thing. Why queueable
at all? Queueable
should be sufficient.
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.
because then i can't use put func(item bqueue.Queueable)
as Cannot use type comparable outside a type constraint: interface is (or embeds) comparable
. Made it any for blockfetcher.
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.
any
is bad. You can use two exported types. Indexed
for GetIndex
(and appropriate callback) and current Queueable
for generics.
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.
Indexable? :)
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.
Can be. If it's just GetIndex()
something about indexes fits, yes.
|
||
for height := range chain.CommitteeHistory { | ||
m = smartcontract.GetDefaultHonestNodeCount(chain.GetNumOfCNs(height)) | ||
verification, _ = smartcontract.CreateDefaultMultiSigRedeemScript(vs[:chain.GetNumOfCNs(height)]) |
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.
You don't really care about particular keys, size estimators doesn't need it.
18c8d20
to
0165688
Compare
The queue can operate with any item that implements proper interface. Signed-off-by: Ekaterina Pavlova <[email protected]>
0165688
to
d94eb81
Compare
7419fdd
to
833b783
Compare
Close #3574 Signed-off-by: Ekaterina Pavlova <[email protected]>
Signed-off-by: Ekaterina Pavlova <[email protected]>
833b783
to
1ad62ef
Compare
Close #3574