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

Update zcbenchmarks to include sapling data #3657

Merged
merged 12 commits into from Feb 26, 2019

Conversation

@Eirik0
Copy link
Contributor

commented Nov 7, 2018

Closes #3395

This PR adds a benchmark named trydecryptsaplingnotes which is intended to be similar to trydecryptnotes. It also adds a benchmark incnotewitnessessapling which is similar to incnotewitnesses.

As a side note, while looking for examples to follow I ran in to a fair amount of setup, which I wanted to be able to reuse, repeated across several tests. I pulled some of that logic in to a utility functions and refactored the existing tests using that setup.

@Eirik0 Eirik0 changed the title Update zcbenchmarks to include sapling data Update zcbenchmarks to include sapling data [WIP] Nov 7, 2018

@Eirik0 Eirik0 self-assigned this Nov 7, 2018

@Eirik0 Eirik0 added this to Review Backlog in Arborist Team Nov 7, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2018

⌛️ Trying commit 4f4619e with merge 28601cf...

zkbot added a commit that referenced this pull request Nov 7, 2018

Auto merge of #3657 - Eirik0:3395-sapling-benchmarks, r=<try>
Update zcbenchmarks to include sapling data [WIP]

Closes #3395

This PR adds a benchmark named `trydecryptsaplingnotes` which is intended to be similar to `trydecryptnotes`. In trydecryptnotes, the transaction we use is t->z. In the sapling version I use a z->z transaction because it seemed sufficient and easier to setup.

This PR also updates the benchmark `incnotewitnesses` to include sapling transactions/notes. In this benchmark there is a parameter for the number of transactions to use. For the lack of a better idea, I made it so half of the transactions used are Sprout and half are Sapling. Another option would have been/could be to add a second test to be Sapling specific.

Currently the actual part of `incnotewitnesses` which we are timing seems to happen quickly, but the setup takes so long that it is not hard to get the rpc to timeout. As a result further thought/discussion will be required to figure out how to deal with this.

I still intend to look in to `connectblockslow` as mentioned in the issue which this closes.

As a side note, while looking for examples to follow I ran in to a fair amount of setup, which I wanted to be able to reuse, repeated across several tests. I pulled some of that logic in to a utility functions and refactored the existing tests using that setup.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2018

☀️ Test successful - pr-try
State: approved= try=True

@daira

daira approved these changes Nov 7, 2018

Copy link
Contributor

left a comment

ACK, modulo the test timeout issue.

Show resolved Hide resolved src/utiltest.cpp Outdated
@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

In regards to the benchmark connectblockslow:
I haven't really updated this because it depends on having some files related to block-107134. That being said I'm pretty sure that whatever this benchmark has been doing was not what it was originally intended to do. This is because the class FakeCoinsViewDB was not updated to reflect the changes made for Sapling in the class CCoinsView. Prior to the last commit, FakeCoinsViewDB directly inherited from CCoinsViewDB to take advantage of some of the logic therewithin. I have collapsed the inheritance hierarchy and pulled in that logic making it harder not to update it in the future.

Not fully understanding this benchmark, I am wondering if it should be updated to depend on a newer block.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

I have left [WIP] in the title because as of right now I am not totally sure what to do about the timeout issue.

@daira

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

ACK 6855342, but I suggest reverting b791d97.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@daira

ACK 6855342, but I suggest reverting b791d97.

I think it was correct to remove this because it is possible to run these rpc-based benchmarks on live nodes. I am not sure what the effect of changing the consensus params (e.g., UpdateNetworkUpgradeParameters(Consensus::UPGRADE_SAPLING, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT);) would be, but my thought was that it could be undesirable.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

I discussed the timeout issue with @str4d and pushed some changes based on his suggestions. Unlike in the Sprout case, it is not possible to create a dummy Sapling transaction without also constructing the proofs as this is not supported by the ffi. For this reason, I changed the txs to be t->z rather than z->z because then we save a couple of seconds not having to generate a proof for the input. I also split the increment note witness benchmark in to one for sprout and one for sapling rather than using a mix of Sprout and Sapling transactions. The idea is that we can run the two benchmarks separately and then interpolate based on those.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Nov 13, 2018

⌛️ Trying commit 2e3d6da with merge 880b9e9...

zkbot added a commit that referenced this pull request Nov 13, 2018

Auto merge of #3657 - Eirik0:3395-sapling-benchmarks, r=<try>
Update zcbenchmarks to include sapling data [WIP]

Closes #3395

This PR adds a benchmark named `trydecryptsaplingnotes` which is intended to be similar to `trydecryptnotes`. In trydecryptnotes, the transaction we use is t->z. In the sapling version I use a z->z transaction because it seemed sufficient and easier to setup.

This PR also updates the benchmark `incnotewitnesses` to include sapling transactions/notes. In this benchmark there is a parameter for the number of transactions to use. For the lack of a better idea, I made it so half of the transactions used are Sprout and half are Sapling. Another option would have been/could be to add a second test to be Sapling specific.

Currently the actual part of `incnotewitnesses` which we are timing seems to happen quickly, but the setup takes so long that it is not hard to get the rpc to timeout. As a result further thought/discussion will be required to figure out how to deal with this.

~I still intend to look in to `connectblockslow` as mentioned in the issue which this closes.~

As a side note, while looking for examples to follow I ran in to a fair amount of setup, which I wanted to be able to reuse, repeated across several tests. I pulled some of that logic in to a utility functions and refactored the existing tests using that setup.

@Eirik0 Eirik0 changed the title Update zcbenchmarks to include sapling data [WIP] Update zcbenchmarks to include sapling data Nov 13, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Nov 13, 2018

☀️ Test successful - pr-try
State: approved= try=True

@daira

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

@Eirik0 wrote:

@daira

ACK 6855342, but I suggest reverting b791d97.

I think it was correct to remove this because it is possible to run these rpc-based benchmarks on live nodes.

Eeek! That sounds dangerous. In that case all of these benchmarks should be reviewed to check that they don't cause undesirable side effects.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@daira In this case I was removing something which I had previously added. I should have perhaps amended the original commit to make it more clear what was going on.

I did go through and have a bit of a look (in zcbenchmarks.cpp) at what benchmarks change consensus parameters and the only one I could see is connectblockslow which calls SelectParams(CBaseChainParams::MAIN); and SelectParamsFromCommandLine().

That being said, following benchmarks (including the above) "must be run in regtest mode" (according to errors thrown in rpcwallet.cpp): connectblockslow, sendtoaddress, loadwallet.

@Eirik0 Eirik0 requested a review from bitcartel Nov 15, 2018

@Eirik0 Eirik0 moved this from Review Backlog to In Review in Arborist Team Nov 28, 2018

@mms710 mms710 moved this from In Review to Merge Queue in Arborist Team Dec 10, 2018

@mms710 mms710 moved this from Merge Queue to In Review in Arborist Team Dec 10, 2018

@mms710 mms710 moved this from In Review to Review Backlog in Arborist Team Dec 20, 2018

@mms710 mms710 moved this from Review Backlog to Current Sprint in Arborist Team Dec 20, 2018

@mms710 mms710 moved this from Current Sprint to PRs That Need Review + Their Associated Issues in Arborist Team Dec 20, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

@Eirik0 need rebase

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2019

⌛️ Trying commit 8a1d193 with merge 6669a8f...

zkbot added a commit that referenced this pull request Jan 30, 2019

Auto merge of #3657 - Eirik0:3395-sapling-benchmarks, r=<try>
Update zcbenchmarks to include sapling data

Closes #3395

This PR adds a benchmark named `trydecryptsaplingnotes` which is intended to be similar to `trydecryptnotes`. It also adds a benchmark `incnotewitnessessapling` which is similar to `incnotewitnesses`.

As a side note, while looking for examples to follow I ran in to a fair amount of setup, which I wanted to be able to reuse, repeated across several tests. I pulled some of that logic in to a utility functions and refactored the existing tests using that setup.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2019

☀️ Test successful - pr-try
State: approved= try=True

@mms710 mms710 moved this from In Review to Current Sprint in Arborist Team Jan 31, 2019

@mms710 mms710 moved this from Current Sprint to Sapling Priorites in Arborist Team Jan 31, 2019

@mms710 mms710 moved this from Sapling Priorites to Sprint Backlog in Arborist Team Jan 31, 2019

@daira

daira approved these changes Feb 21, 2019

Copy link
Contributor

left a comment

re-utACK

@str4d str4d added the benchmarking label Feb 22, 2019

@str4d str4d self-requested a review Feb 22, 2019

@str4d

str4d approved these changes Feb 22, 2019

Copy link
Contributor

left a comment

utACK

@str4d str4d added this to the v2.0.4 milestone Feb 22, 2019

@str4d str4d moved this from Sprint Backlog to Merge Queue in Arborist Team Feb 22, 2019

@str4d

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

📌 Commit 8a1d193 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

⌛️ Testing commit 8a1d193 with merge 36dc562...

zkbot added a commit that referenced this pull request Feb 22, 2019

Auto merge of #3657 - Eirik0:3395-sapling-benchmarks, r=str4d
Update zcbenchmarks to include sapling data

Closes #3395

This PR adds a benchmark named `trydecryptsaplingnotes` which is intended to be similar to `trydecryptnotes`. It also adds a benchmark `incnotewitnessessapling` which is similar to `incnotewitnesses`.

As a side note, while looking for examples to follow I ran in to a fair amount of setup, which I wanted to be able to reuse, repeated across several tests. I pulled some of that logic in to a utility functions and refactored the existing tests using that setup.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

⌛️ Testing commit 8a1d193 with merge 52fbc1c...

zkbot added a commit that referenced this pull request Feb 26, 2019

Auto merge of #3657 - Eirik0:3395-sapling-benchmarks, r=str4d
Update zcbenchmarks to include sapling data

Closes #3395

This PR adds a benchmark named `trydecryptsaplingnotes` which is intended to be similar to `trydecryptnotes`. It also adds a benchmark `incnotewitnessessapling` which is similar to `incnotewitnesses`.

As a side note, while looking for examples to follow I ran in to a fair amount of setup, which I wanted to be able to reuse, repeated across several tests. I pulled some of that logic in to a utility functions and refactored the existing tests using that setup.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 52fbc1c to master...

@zkbot zkbot merged commit 8a1d193 into zcash:master Feb 26, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) Feb 26, 2019

@rex4539

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

This commit broke the compilation on macOS -> #3860

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.