-
Notifications
You must be signed in to change notification settings - Fork 36
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
Commit batcher #584
base: master
Are you sure you want to change the base?
Commit batcher #584
Conversation
Signed-off-by: Markuu-s <qwert11223381@gmail.com>
Signed-off-by: Markuu-s <qwert11223381@gmail.com>
Signed-off-by: Markuu-s <qwert11223381@gmail.com>
Signed-off-by: Markuu-s <qwert11223381@gmail.com>
Signed-off-by: Markuu-s <qwert11223381@gmail.com>
Signed-off-by: Markuu-s <qwert11223381@gmail.com>
Signed-off-by: Markuu-s <qwert11223381@gmail.com>
using fc::mining::types::FeeConfig; | ||
using fc::mining::types::PaddedPieceSize; | ||
using fc::mining::types::Piece; | ||
using fc::mining::types::PieceInfo; |
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.
using fc::mining::types::FeeConfig; | |
using fc::mining::types::PaddedPieceSize; | |
using fc::mining::types::Piece; | |
using fc::mining::types::PieceInfo; | |
using types::FeeConfig; | |
using types::PaddedPieceSize; | |
using types::Piece; | |
using types::PieceInfo; |
#pragma once | ||
#include "api/full_node/node_api.hpp" |
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.
#pragma once | |
#include "api/full_node/node_api.hpp" | |
#pragma once | |
#include "api/full_node/node_api.hpp" |
#pragma once | ||
#include "commit_batcher_impl.hpp" |
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.
#pragma once | |
#include "commit_batcher_impl.hpp" | |
#include "commit_batcher_impl.hpp" |
using fc::BytesIn; | ||
using fc::proofs::ProofEngine; |
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.
using fc::BytesIn; | |
using fc::proofs::ProofEngine; | |
using proofs::ProofEngine; |
#pragma once | ||
#include <libp2p/basic/scheduler.hpp> |
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.
#pragma once | |
#include <libp2p/basic/scheduler.hpp> | |
#pragma once | |
#include <libp2p/basic/scheduler.hpp> |
const SectorNumber §or_number, const TipsetKey &tip_set_key); | ||
}; | ||
|
||
} // namespace fc::mining |
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.
missed empty line
|
||
void forceSend() override; | ||
|
||
void setCommitCutoff(const ChainEpoch ¤t_epoch, |
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.
Probably we want to make it private
OUTCOME_TRY(tipset, api_->ChainGetTipSet(head->key)); | ||
const BigInt base_fee = tipset->blks[0].parent_base_fee; | ||
|
||
TokenAmount agg_fee_raw = AggregateProveCommitNetworkFee( |
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.
TokenAmount agg_fee_raw = AggregateProveCommitNetworkFee( | |
const TokenAmount agg_fee_raw = AggregateProveCommitNetworkFee( |
TokenAmount agg_fee = bigdiv(agg_fee_raw * agg_fee_num_, agg_fee_den_); | ||
TokenAmount need_funds = collateral + agg_fee; | ||
TokenAmount good_funds = max_fee + need_funds; |
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.
TokenAmount agg_fee = bigdiv(agg_fee_raw * agg_fee_num_, agg_fee_den_); | |
TokenAmount need_funds = collateral + agg_fee; | |
TokenAmount good_funds = max_fee + need_funds; | |
const TokenAmount agg_fee = bigdiv(agg_fee_raw * agg_fee_num_, agg_fee_den_); | |
const TokenAmount need_funds = collateral + agg_fee; | |
const TokenAmount good_funds = max_fee + need_funds; |
const SectorInfo §or_info) { | ||
ChainEpoch cutoff_epoch = | ||
sector_info.ticket_epoch | ||
+ static_cast<int64_t>(kEpochsInDay + kChainFinality); |
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.
+ static_cast<int64_t>(kEpochsInDay + kChainFinality); | |
+ static_cast<ChainEpoch>(kEpochsInDay + kChainFinality); |
struct AggregateInput { | ||
Proof proof; | ||
AggregateSealVerifyInfo info; | ||
RegisteredSealProof spt; |
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.
RegisteredSealProof spt; | |
RegisteredSealProof seal_proof_type{RegisteredSealProof::kUndefined}; |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
#pragma once |
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.
#pragma once |
Not in .cpp file.
collateral = collateral + pci.precommit_deposit; | ||
collateral = std::max(BigInt(0), collateral); | ||
|
||
return collateral; |
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.
return collateral; | |
return std::move(collateral); |
const BigInt base_fee = tipset->blks[0].parent_base_fee; | ||
|
||
TokenAmount agg_fee_raw = AggregateProveCommitNetworkFee( | ||
infos.size(), base_fee); // TODO change to aggregateNetworkFee |
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.
infos.size(), base_fee); // TODO change to aggregateNetworkFee | |
infos.size(), base_fee); // TODO((Markuuu-s) change to aggregateNetworkFee |
} | ||
|
||
const ActorId mid = miner_address_.getId(); | ||
// TODO maybe long (AggregateSealProofs) |
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.
// TODO maybe long (AggregateSealProofs) | |
// TODO(Markuuu-s) maybe long (AggregateSealProofs) |
return lhs.publish_cid == rhs.publish_cid && lhs.deal_id == rhs.deal_id | ||
&& lhs.deal_schedule == rhs.deal_schedule | ||
&& lhs.is_keep_unsealed == rhs.is_keep_unsealed; | ||
if (lhs.is_keep_unsealed == rhs.is_keep_unsealed) { |
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 is the difference? Please, revert.
const BigInt agg_fee_num_ = BigInt(110); | ||
const BigInt agg_fee_den_ = BigInt(100); | ||
const RegisteredAggregationProof arp_ = RegisteredAggregationProof(0); |
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.
Please, use const naming for constants.
using BatcherCallbackMock = | ||
std::function<void(const outcome::result<CID> &cid)>; |
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.
using BatcherCallbackMock = | |
std::function<void(const outcome::result<CID> &cid)>; |
It is a fc::mining::CommitCallback
ActorId miner_id_; | ||
std::shared_ptr<ProofEngineMock> proof_; | ||
std::shared_ptr<FeeConfig> fee_config_; | ||
BatcherCallbackMock callback_mock_; |
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.
BatcherCallbackMock callback_mock_; | |
CommitCallback callback_mock_; |
#pragma once | ||
#include "const.hpp" |
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.
#pragma once | |
#include "const.hpp" | |
#pragma once | |
#include "const.hpp" |
*/ | ||
|
||
#pragma once | ||
#include "commit_batcher_impl.hpp" |
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.
Please use full path
|
||
void CommitBatcherImpl::reschedule(std::chrono::milliseconds time) { | ||
handle_ = scheduler_->scheduleWithHandle( | ||
[&]() { |
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.
Could be segfault
outcome::result<CID> CommitBatcherImpl::sendBatch( | ||
const MapPairStorage &pair_storage_for_send) { | ||
if (pair_storage_for_send.empty()) { | ||
cutoff_start_ = std::chrono::system_clock::now(); |
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 remove it, because after sendBatch call we set it to now
const MapPairStorage &pair_storage_for_send) { | ||
if (pair_storage_for_send.empty()) { | ||
cutoff_start_ = std::chrono::system_clock::now(); | ||
return ERROR_TEXT("Empty Batcher"); |
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 it error, maybe just outcome::success()?
proofsSpan.push_back(gsl::make_span(proof)); | ||
} | ||
|
||
OUTCOME_TRY(proof_->aggregateSealProofs(aggregate_seal, proofsSpan)); |
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.
OUTCOME_TRY(proof_->aggregateSealProofs(aggregate_seal, proofsSpan)); | |
// TODO(Markuuu-s) maybe long (AggregateSealProofs) | |
OUTCOME_TRY(proof_->aggregateSealProofs(aggregate_seal, proofsSpan)); |
} | ||
|
||
const ActorId mid = miner_address_.getId(); | ||
// TODO maybe long (AggregateSealProofs) |
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.
// TODO maybe long (AggregateSealProofs) |
for (const Proof &proof : proofs) { | ||
proofsSpan.push_back(gsl::make_span(proof)); | ||
} |
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.
proofs
is redundant. You could fill proofsSpan
directly
MethodParams{encode}), | ||
kPushNoSpec)); | ||
|
||
cutoff_start_ = std::chrono::system_clock::now(); |
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.
cutoff_start_ = std::chrono::system_clock::now(); |
const TokenAmount effectiveGasFee = std::max(base_fee, kBatchBalancer); | ||
const TokenAmount networkFeeNum = | ||
effectiveGasFee * kEstimatedSinglePreCommitGasUsage * aggregate_size | ||
* kBatchDiscountNumerator; | ||
return bigdiv(networkFeeNum, kBatchDiscountDenominator); |
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.
const TokenAmount effectiveGasFee = std::max(base_fee, kBatchBalancer); | |
const TokenAmount networkFeeNum = | |
effectiveGasFee * kEstimatedSinglePreCommitGasUsage * aggregate_size | |
* kBatchDiscountNumerator; | |
return bigdiv(networkFeeNum, kBatchDiscountDenominator); | |
return aggregateNetworkFee(aggregate_size, kEstimatedSinglePreCommitGasUsage, base_fee); |
ActorId miner_id_; | ||
std::shared_ptr<ProofEngineMock> proof_; | ||
std::shared_ptr<FeeConfig> fee_config_; | ||
BatcherCallbackMock callback_mock_; |
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.
BatcherCallbackMock callback_mock_; | |
MockStdFunction<CommitCallback> callback_mock_; |
/** | ||
* @given 2 commits and max_size_callback is 2 | ||
* @when send the 2 commits | ||
* @then the result should be 2 messages in message pool with pair of commits |
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.
* @then the result should be 2 messages in message pool with pair of commits | |
* @then the result should be 1 message in message pool with 2 commits |
} | ||
|
||
/** | ||
* @given 4 commits |
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.
There are 3 commits currently: 1 before first timeout, and two after
fee_config_(std::move(fee_config)), | ||
proof_(std::move(proof)), | ||
address_selector_(std::move(address_selector)) { | ||
cutoff_start_ = std::chrono::system_clock::now(); |
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.
maybe use scheduler
cutoff_start_ = std::chrono::system_clock::now(); | |
cutoff_start_ = scheduler_->now(); |
const SectorInfo §or_info) { | ||
ChainEpoch cutoff_epoch = | ||
sector_info.ticket_epoch | ||
+ static_cast<int64_t>(kEpochsInDay + kChainFinality); |
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 maybe 30*
.
may move to miner policy header
forceSend(); | ||
} else { | ||
const auto temp_cutoff = std::chrono::milliseconds( | ||
(cutoff_epoch - current_epoch) * kEpochDurationSeconds); |
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.
(cutoff_epoch - current_epoch) * kEpochDurationSeconds); | |
(cutoff_epoch - current_epoch) * kBlockDelaySecs); |
api_->StateMinerInitialPledgeCollateral( | ||
miner_address_, pci.info, tip_set_key)); | ||
|
||
collateral = collateral + pci.precommit_deposit; |
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.
collateral = collateral + pci.precommit_deposit; | |
collateral = collateral - pci.precommit_deposit; |
cutoff_start_ = std::chrono::system_clock::now(); | ||
closest_cutoff_ = max_delay_; | ||
reschedule(max_delay_); |
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.
maybe better to store queued commits with cutoff, sorted by cutoff, and reschedule by minimal cutoff from queue
|
||
const BigInt agg_fee_num_ = BigInt(110); | ||
const BigInt agg_fee_den_ = BigInt(100); | ||
const RegisteredAggregationProof arp_ = RegisteredAggregationProof(0); |
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.
const RegisteredAggregationProof arp_ = RegisteredAggregationProof(0); | |
const RegisteredAggregationProof arp_ = RegisteredAggregationProof::SnarkPackV1; |
Description of the Change
Benefits
Possible Drawbacks
Usage Examples or Tests [optional]
Alternate Designs [optional]