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

cryptonote_core: skip block notify on blockchain switching rollback #6347

Merged
merged 1 commit into from Apr 4, 2020

Conversation

@xiphon
Copy link
Contributor

xiphon commented Feb 19, 2020

No description provided.

@moneromooo-monero

This comment has been minimized.

Copy link
Collaborator

moneromooo-monero commented Feb 19, 2020

Presumably you already got notifications for the blocks in the alt chain that untimately failed, right ? If so, it seems to make more sense to not omit those notifications ?

@xiphon xiphon force-pushed the xiphon:rollback-block-notify branch from e0e8475 to fcb06f7 Feb 19, 2020
@xiphon

This comment has been minimized.

Copy link
Contributor Author

xiphon commented Feb 19, 2020

Updated.

  1. Failed to switch to alt chain - don't send any notification
  2. Successfully switched to alt chain - send 1) reorg notification and 2) block notification for each alt chain block
@@ -1044,6 +1044,11 @@ bool Blockchain::switch_to_alternative_blockchain(std::list<block_extended_info>
reorg_notify->notify("%s", std::to_string(split_height).c_str(), "%h", std::to_string(m_db->height()).c_str(),
"%n", std::to_string(m_db->height() - split_height).c_str(), "%d", std::to_string(discarded_blocks).c_str(), NULL);

std::shared_ptr<tools::Notify> block_notify = m_block_notify;

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 1, 2020

Contributor

I don't see any benefit to copying the std::shared_ptr in the original code. The copy is not atomic/thread-safe (with respect to assignment of the same std::shared_ptr object) unless the appropriate functions are used. Was std::atomic_store and std::atomic_load intended to be used here?

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 1, 2020

Contributor

Sorry ping @moneromooo-monero

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Mar 1, 2020

Collaborator

Yes, I thought the pointer copy was atomic.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 4, 2020

Contributor

I think the risks of issue here is low due to the init process. But something to remember for sure.

@luigi1111 luigi1111 merged commit 4454700 into monero-project:master Apr 4, 2020
5 checks passed
5 checks passed
build-macos
Details
build-windows
Details
build-ubuntu
Details
test-ubuntu
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.