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

Feature/candidate baking #1307

Merged
merged 7 commits into from
Sep 19, 2022
Merged

Feature/candidate baking #1307

merged 7 commits into from
Sep 19, 2022

Conversation

iceseer
Copy link
Contributor

@iceseer iceseer commented Aug 9, 2022

Referenced issues

Resolves #1289

Description of the Change

Candidate baking implementation for parachains.

@iceseer iceseer force-pushed the feature/candidate_baking branch from b093311 to 9d22ae2 Compare August 10, 2022 22:16
@iceseer iceseer force-pushed the feature/candidate_baking branch from 35b21b3 to 50ed8d0 Compare August 31, 2022 11:13
Signed-off-by: iceseer <iceseer@gmail.com>
@iceseer iceseer force-pushed the feature/candidate_baking branch from 50ed8d0 to 2169b6c Compare August 31, 2022 11:16
@iceseer iceseer marked this pull request as ready for review August 31, 2022 11:23
@iceseer iceseer requested review from kamilsa and turuslan August 31, 2022 11:23
Signed-off-by: iceseer <iceseer@gmail.com>
Signed-off-by: iceseer <iceseer@gmail.com>
@iceseer iceseer force-pushed the feature/candidate_baking branch from 06137e7 to 2cdaa39 Compare September 1, 2022 12:40
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #1307 (9552195) into master (9512e5b) will decrease coverage by 0.43%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1307      +/-   ##
==========================================
- Coverage   24.94%   24.50%   -0.44%     
==========================================
  Files         612      612              
  Lines       22589    22984     +395     
  Branches    11794    11998     +204     
==========================================
- Hits         5635     5633       -2     
- Misses      11751    12149     +398     
+ Partials     5203     5202       -1     
Impacted Files Coverage Δ
core/injector/application_injector.cpp 32.12% <0.00%> (-0.21%) ⬇️
core/network/impl/peer_manager_impl.cpp 3.20% <0.00%> (+0.02%) ⬆️
core/network/impl/protocols/collation_protocol.cpp 0.00% <0.00%> (ø)
core/network/impl/protocols/collation_protocol.hpp 0.00% <0.00%> (ø)
core/network/impl/protocols/protocol_base_impl.hpp 0.00% <0.00%> (ø)
core/network/impl/protocols/protocol_error.cpp 0.00% <0.00%> (ø)
...twork/impl/protocols/request_response_protocol.hpp 0.00% <0.00%> (ø)
core/network/impl/stream_engine.hpp 23.50% <ø> (ø)
core/network/peer_manager.hpp 50.00% <ø> (ø)
...re/parachain/validator/impl/parachain_observer.cpp 0.00% <0.00%> (ø)
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@iceseer iceseer requested a review from turuslan September 1, 2022 13:32
turuslan
turuslan previously approved these changes Sep 5, 2022
core/parachain/validator/parachain_processor.hpp Outdated Show resolved Hide resolved
Comment on lines +291 to +293
std::shared_ptr<WorkersContext> context_;
std::shared_ptr<WorkGuard> work_guard_;
std::unique_ptr<std::thread> workers_[kBackgroundWorkers];
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to wrap as (?)ThreadPool for reuse.

core/parachain/validator/parachain_processor.hpp Outdated Show resolved Hide resolved
core/parachain/validator/parachain_processor.hpp Outdated Show resolved Hide resolved
core/parachain/validator/impl/parachain_processor.cpp Outdated Show resolved Hide resolved
@iceseer iceseer requested review from ortyomka and removed request for kamilsa September 13, 2022 09:21
core/injector/application_injector.cpp Outdated Show resolved Hide resolved
core/network/peer_manager.hpp Outdated Show resolved Hide resolved
core/parachain/validator/impl/parachain_processor.cpp Outdated Show resolved Hide resolved
iceseer and others added 3 commits September 16, 2022 09:21
Signed-off-by: iceseer <iceseer@gmail.com>
Signed-off-by: iceseer <iceseer@gmail.com>
@ortyomka ortyomka self-requested a review September 16, 2022 07:24
@turuslan turuslan dismissed their stale review September 16, 2022 07:26

New changes

func{std::forward<F>(func)},
stream](auto &&result) mutable {
auto self = wptr.lock();
if (!result || !self) return std::forward<F>(func)(std::move(result));
Copy link
Contributor

@turuslan turuslan Sep 16, 2022

Choose a reason for hiding this comment

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

Suggested change
if (!result || !self) return std::forward<F>(func)(std::move(result));
if (!result || !self) {
return std::forward<F>(func)(std::move(result));

});
template <bool DirectionIncoming, typename F>
void doCollatorHandshake(
std::shared_ptr<kagome::network::Stream> const &stream, F &&func) {
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
std::shared_ptr<kagome::network::Stream> const &stream, F &&func) {
const std::shared_ptr<kagome::network::Stream> &stream, F &&func) {

Comment on lines +69 to +72
if (!stream)
cb(ProtocolError::HANDSHAKE_ERROR);
else
cb(stream);
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
if (!stream)
cb(ProtocolError::HANDSHAKE_ERROR);
else
cb(stream);
if (!stream) {
cb(ProtocolError::HANDSHAKE_ERROR);
} else {
cb(stream);
}

doCollatorHandshake<true>(
stream,
[wptr{weak_from_this()}](
std::shared_ptr<Stream> const &stream) mutable {
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
std::shared_ptr<Stream> const &stream) mutable {
const std::shared_ptr<Stream> &stream) mutable {

@@ -305,6 +304,7 @@ namespace kagome::network {
}

ProtocolBaseImpl base_;
Protocol const protocol_;
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
Protocol const protocol_;
const Protocol protocol_;

Comment on lines +122 to +127
auto const &[it, inserted] = peer_map.insert(
std::make_pair(peer_id,
std::make_shared<FetchedCollationState>(
std::move(response),
CollationState::kFetched,
PoVDataState::kComplete)));
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
auto const &[it, inserted] = peer_map.insert(
std::make_pair(peer_id,
std::make_shared<FetchedCollationState>(
std::move(response),
CollationState::kFetched,
PoVDataState::kComplete)));
auto inserted = peer_map.emplace(
peer_id,
std::make_shared<FetchedCollationState>(
std::move(response),
CollationState::kFetched,
PoVDataState::kComplete)).second;

Comment on lines +244 to +255
outcome::result<network::Signature> ParachainProcessorImpl::sign(
T const &t) const {
if (!keypair_) return Error::KEY_NOT_PRESENT;

auto payload = scale::encode(t).value();
return crypto_provider_->sign(*keypair_, payload).value();
}

std::optional<network::ValidatorIndex> ParachainProcessorImpl::getOurIndex() {
logger_->error("Not implemented. Return my validator index.");
return std::nullopt;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

May use ValidatorSigner from #1336 (TODO: signable trait or conversion function, e.g. bitfield is encoded as is, but statement has prefix and conversion to other type)

candidateDescriptorFrom(*validation_result.fetched_collation),
.commitments = std::move(*validation_result.commitments)};

auto sign_result = sign(receipt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Must sign ("BKNG": [u8; 4], 1: u8, candidate_hash: [u8; 32])
CompactStatementInner

} else if constexpr (kStatementType == StatementType::kValid) {
auto const candidate_hash =
candidateHashFrom(*validation_result.fetched_collation);
auto sign_result = sign(candidate_hash);
Copy link
Contributor

Choose a reason for hiding this comment

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

Must sign ("BKNG": [u8; 4], 2: u8, candidate_hash: [u8; 32])

core/parachain/validator/impl/parachain_processor.cpp Outdated Show resolved Hide resolved
Signed-off-by: iceseer <iceseer@gmail.com>
@iceseer iceseer force-pushed the feature/candidate_baking branch from b7a7f26 to 9552195 Compare September 19, 2022 06:05
@iceseer iceseer merged commit 3834e5a into master Sep 19, 2022
@iceseer iceseer deleted the feature/candidate_baking branch September 19, 2022 07:35
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.

Candidate backing
3 participants