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

Commit batcher #584

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Commit batcher #584

wants to merge 11 commits into from

Conversation

Markuu-s
Copy link
Collaborator

@Markuu-s Markuu-s commented Feb 8, 2022

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

Signed-off-by: Markuu-s <[email protected]>
Comment on lines +30 to +33
using fc::mining::types::FeeConfig;
using fc::mining::types::PaddedPieceSize;
using fc::mining::types::Piece;
using fc::mining::types::PieceInfo;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;

Comment on lines +6 to +7
#pragma once
#include "api/full_node/node_api.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#pragma once
#include "api/full_node/node_api.hpp"
#pragma once
#include "api/full_node/node_api.hpp"

Comment on lines +6 to +7
#pragma once
#include "commit_batcher_impl.hpp"
Copy link
Collaborator

@wer1st wer1st Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#pragma once
#include "commit_batcher_impl.hpp"
#include "commit_batcher_impl.hpp"

Comment on lines +17 to +18
using fc::BytesIn;
using fc::proofs::ProofEngine;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using fc::BytesIn;
using fc::proofs::ProofEngine;
using proofs::ProofEngine;

Comment on lines +6 to +7
#pragma once
#include <libp2p/basic/scheduler.hpp>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#pragma once
#include <libp2p/basic/scheduler.hpp>
#pragma once
#include <libp2p/basic/scheduler.hpp>

const SectorNumber &sector_number, const TipsetKey &tip_set_key);
};

} // namespace fc::mining
Copy link
Collaborator

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 &current_epoch,
Copy link
Collaborator

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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TokenAmount agg_fee_raw = AggregateProveCommitNetworkFee(
const TokenAmount agg_fee_raw = AggregateProveCommitNetworkFee(

Comment on lines +177 to +179
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 &sector_info) {
ChainEpoch cutoff_epoch =
sector_info.ticket_epoch
+ static_cast<int64_t>(kEpochsInDay + kChainFinality);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+ static_cast<int64_t>(kEpochsInDay + kChainFinality);
+ static_cast<ChainEpoch>(kEpochsInDay + kChainFinality);

struct AggregateInput {
Proof proof;
AggregateSealVerifyInfo info;
RegisteredSealProof spt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RegisteredSealProof spt;
RegisteredSealProof seal_proof_type{RegisteredSealProof::kUndefined};

* SPDX-License-Identifier: Apache-2.0
*/

#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#pragma once

Not in .cpp file.

collateral = collateral + pci.precommit_deposit;
collateral = std::max(BigInt(0), collateral);

return collateral;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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) {
Copy link
Contributor

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.

Comment on lines +70 to +72
const BigInt agg_fee_num_ = BigInt(110);
const BigInt agg_fee_den_ = BigInt(100);
const RegisteredAggregationProof arp_ = RegisteredAggregationProof(0);
Copy link
Contributor

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.

Comment on lines +40 to +41
using BatcherCallbackMock =
std::function<void(const outcome::result<CID> &cid)>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BatcherCallbackMock callback_mock_;
CommitCallback callback_mock_;

Comment on lines +6 to +7
#pragma once
#include "const.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#pragma once
#include "const.hpp"
#pragma once
#include "const.hpp"

*/

#pragma once
#include "commit_batcher_impl.hpp"
Copy link
Contributor

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(
[&]() {
Copy link
Contributor

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();
Copy link
Contributor

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");
Copy link
Contributor

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO maybe long (AggregateSealProofs)

Comment on lines +158 to +160
for (const Proof &proof : proofs) {
proofsSpan.push_back(gsl::make_span(proof));
}
Copy link
Contributor

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cutoff_start_ = std::chrono::system_clock::now();

core/vm/actor/builtin/v6/monies.hpp Show resolved Hide resolved
Comment on lines +20 to +24
const TokenAmount effectiveGasFee = std::max(base_fee, kBatchBalancer);
const TokenAmount networkFeeNum =
effectiveGasFee * kEstimatedSinglePreCommitGasUsage * aggregate_size
* kBatchDiscountNumerator;
return bigdiv(networkFeeNum, kBatchDiscountDenominator);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BatcherCallbackMock callback_mock_;
MockStdFunction<CommitCallback> callback_mock_;

example

/**
* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @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
Copy link
Contributor

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();
Copy link
Contributor

@turuslan turuslan Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use scheduler

Suggested change
cutoff_start_ = std::chrono::system_clock::now();
cutoff_start_ = scheduler_->now();

const SectorInfo &sector_info) {
ChainEpoch cutoff_epoch =
sector_info.ticket_epoch
+ static_cast<int64_t>(kEpochsInDay + kChainFinality);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forceSend();
} else {
const auto temp_cutoff = std::chrono::milliseconds(
(cutoff_epoch - current_epoch) * kEpochDurationSeconds);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(cutoff_epoch - current_epoch) * kEpochDurationSeconds);
(cutoff_epoch - current_epoch) * kBlockDelaySecs);

api_->StateMinerInitialPledgeCollateral(
miner_address_, pci.info, tip_set_key));

collateral = collateral + pci.precommit_deposit;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
collateral = collateral + pci.precommit_deposit;
collateral = collateral - pci.precommit_deposit;

Comment on lines +85 to +87
cutoff_start_ = std::chrono::system_clock::now();
closest_cutoff_ = max_delay_;
reschedule(max_delay_);
Copy link
Contributor

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const RegisteredAggregationProof arp_ = RegisteredAggregationProof(0);
const RegisteredAggregationProof arp_ = RegisteredAggregationProof::SnarkPackV1;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants