-
Notifications
You must be signed in to change notification settings - Fork 23
perf(blockassembly): optimize capacity management with early validation checks #455
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,8 @@ import ( | |
| "go.uber.org/atomic" | ||
| "golang.org/x/sync/errgroup" | ||
| "google.golang.org/grpc" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| "google.golang.org/protobuf/types/known/timestamppb" | ||
| ) | ||
|
|
||
|
|
@@ -890,6 +892,13 @@ func (ba *BlockAssembly) AddTx(ctx context.Context, req *blockassembly_api.AddTx | |
| } | ||
|
|
||
| if !ba.settings.BlockAssembly.Disabled { | ||
| if !ba.blockAssembler.subtreeProcessor.CanAcceptTransactions(1) { | ||
| return nil, status.Errorf(codes.ResourceExhausted, | ||
| "capacity limit reached: current=%d, max=%d", | ||
| ba.blockAssembler.subtreeProcessor.CurrentTransactionCount(), | ||
| ba.blockAssembler.subtreeProcessor.GetMaxUnminedTransactions()) | ||
| } | ||
|
|
||
| ba.blockAssembler.AddTxBatch( | ||
| []subtreepkg.Node{{Hash: chainhash.Hash(req.Txid), Fee: req.Fee, SizeInBytes: req.Size}}, | ||
| []*subtreepkg.TxInpoints{&txInpoints}, | ||
|
|
@@ -1000,6 +1009,13 @@ func (ba *BlockAssembly) AddTxBatch(ctx context.Context, batch *blockassembly_ap | |
|
|
||
| // Add entire batch in one call | ||
| if !ba.settings.BlockAssembly.Disabled { | ||
| if !ba.blockAssembler.subtreeProcessor.CanAcceptTransactions(len(nodes)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same Race Condition (Amplified by Batch Size) This has the same check-then-act race as AddTx. With batches, the overage could be significant - if the limit is 1000 and two threads each submit batches of 500 when current=600, both could pass the check and push the total to 1600 (60% over limit). The dual-check pattern (here + AddBatch) narrows the window but does not eliminate the race between CanAcceptTransactions and enqueueBatch. Resolution: This is acceptable for this use case. The limit is a soft operational safety measure to prevent OOM during restarts, not a hard security boundary. Even with batch amplification, the overage is bounded by the number of concurrent batch submissions, which is limited by server concurrency. The protection still significantly reduces the risk of OOM compared to having no limit. Alternative approaches would add complexity without sufficient benefit for this operational protection mechanism. |
||
| return nil, status.Errorf(codes.ResourceExhausted, | ||
| "capacity limit reached: current=%d, max=%d", | ||
| ba.blockAssembler.subtreeProcessor.CurrentTransactionCount(), | ||
| ba.blockAssembler.subtreeProcessor.GetMaxUnminedTransactions()) | ||
| } | ||
|
|
||
| ba.blockAssembler.AddTxBatch(nodes, txInpointsList) | ||
| } | ||
|
|
||
|
|
@@ -1133,6 +1149,13 @@ func (ba *BlockAssembly) AddTxBatchColumnar(ctx context.Context, req *blockassem | |
|
|
||
| // Add entire batch in one call | ||
| if !ba.settings.BlockAssembly.Disabled { | ||
| if !ba.blockAssembler.subtreeProcessor.CanAcceptTransactions(len(nodes)) { | ||
| return nil, status.Errorf(codes.ResourceExhausted, | ||
| "capacity limit reached: current=%d, max=%d", | ||
| ba.blockAssembler.subtreeProcessor.CurrentTransactionCount(), | ||
| ba.blockAssembler.subtreeProcessor.GetMaxUnminedTransactions()) | ||
| } | ||
|
|
||
| ba.blockAssembler.AddTxBatch(nodes, txInpointsList) | ||
| } | ||
|
|
||
|
|
@@ -1993,3 +2016,33 @@ func (ba *BlockAssembly) SetSkipWaitForPendingBlocks(skip bool) { | |
| ba.blockAssembler.SetSkipWaitForPendingBlocks(skip) | ||
| } | ||
| } | ||
|
|
||
| // CanAcceptTransaction checks if block assembly can accept more transactions. | ||
| // This method is used by the validator to fail fast before spending UTXOs | ||
| // if the capacity limit has been reached. | ||
| // | ||
| // Parameters: | ||
| // - ctx: Context for the operation | ||
| // - req: Request containing the number of transactions to check | ||
| // | ||
| // Returns: | ||
| // - Response with capacity information | ||
| // - error: Any error encountered | ||
| func (ba *BlockAssembly) CanAcceptTransaction(ctx context.Context, req *blockassembly_api.CanAcceptTransactionRequest) (*blockassembly_api.CanAcceptTransactionResponse, error) { | ||
| count := req.Count | ||
| if count == 0 { | ||
| count = 1 | ||
| } | ||
|
|
||
| canAccept := ba.blockAssembler.subtreeProcessor.CanAcceptTransactions(int(count)) | ||
| currentCount := ba.blockAssembler.subtreeProcessor.CurrentTransactionCount() | ||
| maxLimit := ba.blockAssembler.subtreeProcessor.GetMaxUnminedTransactions() | ||
| remainingCapacity := ba.blockAssembler.subtreeProcessor.RemainingCapacity() | ||
|
|
||
| return &blockassembly_api.CanAcceptTransactionResponse{ | ||
| CanAccept: canAccept, | ||
| CurrentCount: currentCount, | ||
| MaxLimit: maxLimit, | ||
| RemainingCapacity: remainingCapacity, | ||
| }, nil | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Race Condition: Check-Then-Act Pattern
The capacity check is not atomic with the subsequent queue operation. While AddBatch provides a second check, the race still exists:
The window is narrow but exists between lines 1707 (CanAcceptTransactions check) and 1716 (enqueueBatch).
Resolution: This is acceptable for this use case. The limit is a soft operational safety measure to prevent OOM during restarts, not a hard security boundary. The worst-case overage is bounded by concurrent request count, which is acceptable. Alternative approaches (atomic compare-and-swap) would add complexity without significant benefit for this operational protection mechanism.