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

Backport of Ryo fix for a wallet caching bug #4247

Merged

Conversation

Projects
None yet
3 participants
@fireice-uk
Copy link
Contributor

commented Aug 10, 2018

@fireice-uk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2018

Note, please see the other commits in the xref'ed pr. I didn't include them because our fork selection system works differently to yours.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

Looks ok, please use a sensible commit message though (eg, "node_rpc_proxy: fix fork earliest height caching" or so).

@fireice-uk fireice-uk force-pushed the fireice-uk:topic-wallet-caching-bug branch from 7a2a429 to c9f0084 Aug 13, 2018

@fireice-uk fireice-uk force-pushed the fireice-uk:topic-wallet-caching-bug branch from c9f0084 to 10475ab Aug 13, 2018

@fireice-uk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

Changed. Please note my previous message, you also need to either:

  • Fix get_earliest_ideal_height_for_version to return an invalid height for unknown forks as per this commit ryo-currency/ryo-currency@12bad0a

  • Cache enabled separately and re-combine the result post-caching.

As this is more of high-level "where do you want to go?" decision I didn't include it. Let me know if you want to go 12bad0a route and I can back port that code too.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

Can you explain why ? If the "unknown" fork is higher than the highest known one, we return max value and never use those rules. If it's between two known ones, we use it when we switch to the next one, so that looks OK to me at first glance.

@fireice-uk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2018

Problem is that get_earliest_ideal_height_for_version will return 1546000 for any input >= 7 in your case. As long as that behaviour is expected then you can get away with just this patch.

In our case we have different features enabled on different versions for the testnet and mainnet so this is how it triggered a secondary bug (we assigned bulletproofs to a disabled v255).

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

OK. It should currently return max uint64_t by my reading, and it changed a few months ago to support "jumps" in versions as aeon was doing that and this got backported. We'll do with just what you PRed, thanks.

@luigi1111 luigi1111 merged commit 10475ab into monero-project:master Aug 23, 2018

1 of 9 checks passed

buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
buildbot/monero-linux-armv7 Build done.
Details

luigi1111 added a commit that referenced this pull request Aug 23, 2018

Merge pull request #4247
10475ab node_rpc_proxy: fix fork earliest height caching [RYO backport] (fireice-uk)

@stoffu stoffu referenced this pull request Mar 12, 2019

Open

Upstream merge 4 #106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.