Bounty: Fix race conditions causing problems identified in #417 #453
Comments
|
This issue now has a funding of 0.5 ETH (374.81 USD) attached to it.
|
|
happy new year! im going to spend some cycles this week sourcing someone to take this on.. |
|
FYI, ethereumjs/merkle-patricia-tree#28 looks like a promising fix for I dug a little deeper into |
|
Hi, I am the author of ethereumjs/merkle-patricia-tree#28. I also looked at the pop error and I think it's related to reading a non-existent trie node (maybe due to some confusion about the currently active DB). IIRC, the problematic call stack goes like this: // baseTrie.js
put
findPath
_walkTrie
_lookupNode
_walkTrie.processNode
findPath.processNode
_updateNodeWhere If you look at the cb then does Thus, Also interesting to look at are the conditions under which |
|
@federicobond your analysis of the call stack is similar to what I observed. but I don't think the root cause can be fixed in merkle-patricia-tree. I think the root cause pertains to ethereumjs-vm working on its cache of statemanager while in checkpoint mode, then when it tries to persist its cache after certain revert actions, there is a reference to a node that never existed in the db, hence your call stack occurs. |
|
That makes sense. I'll see if I find anything else. |
|
The funding of 0.5 ETH (427.59 USD) attached has been claimed by @kammerdiener. @kammerdiener, please leave a comment to let the funder (@owocki) and the other parties involved your implementation plan. If you don't leave a comment, the funder may expire your claim at their discretion.
|
|
So a quick update on this. Based on what I have found I think that both issues are currently originating from ethereumjs/merkle-patricia-tree. I have fixed the get being undefined. Currently working on figuring out what is going on with pop coming back undefined. |
|
Believe I have fixed it all within ethereumjs/merkle-patricia-tree. The Pull Request is here: PR #30. I tried to take into account all of the information talked about from above and was sure to check @federicobond PR to help fix the _getDBs portion. All of the tests pass now with no errors. |
|
I went into the node_modules folder and replaced the items and am attaching a screenshot of it passing the race conditions test. Once the PR for Merkle Patricia Trees is merged then. Should I go ahead and submit a PR for this with it including the race conditions test (removing the skip command)?
|
|
@kammerdiener there are actually a couple of different competing submissions at this point. I'll be taking some time early this week to evaluate them. I realize now that I said that the goal was to make the tests pass, but the real goal is to fix #417. The best thing that you can do at this point is to manually test ganache-cli with your fix in place (you can use npm link to do this) and determine whether you observe either of the crashes identified in #417. This is how I'll be evaluating the proposed fixes as well, so it'll take a bit of time, I'm afraid. |
|
@roderik by any chance would you like to help with evaluating these fixes, as I believe you've been impacted by this bug quite regularly? |
|
@kammerdiener, as I feared, when testing your fix I observe a new issue. I believe this is due to what @0xNPE @federicobond were discussing above. That said, your changes do make ganache much more stable, so until a proper fix is found we will likely include this fix in future releases (provided that it is accepted by the ethereumjs/merkle-patricia-tree maintainers). The newly observed issue: |
|
Alright thanks for the information. I will start working trying to figure this out as well. Do you have any more information on how you caused this error? |
|
@kammerdiener it's fairly easy to do this with trufflesuite/ganache, as it'll crash somewhat readily when running I recommend using the Also I'd expected ganache-cli to have good sourcemap support by now. I'm looking into this and will commit a fix to the relevant places once that's done. Will comment back here as well, hopefully with a mapped stack trace rather than the useless one I just posted. |
|
@benjamincburns I will be watching for it. Thanks for the tip. I will see what I can do to fix it and make it more stable! |
|
Also be advised that the error I reported will be visible as an error response in your tests. Ganache doesn't crash in this case, but it's also useless for future tests (rerunning tests w/o restarting Ganache just spits out the same error) |
|
@kammerdiener I managed to fix sourcemaps. I'll commit that to the In the mean time, here's the mapped stacktrace: |
|
Actually now that I read that a bit more carefully, it seems that it's actually broken the truffle side of things. I'll dig in a bit further and see if I can't figure out what's gone amiss. |
|
@kammerdiener I'm afraid I also just observed the original issue with your fix in place: |
|
@benjamincburns gladly! |
|
@kammerdiener did you still want to claim the work here, or should i release it to someone else? |
|
@owocki I'll work on it some more. I think I have an idea for the fix. |
|
@owocki I have exhausted all of my potential fixes. If you would go ahead and release it in the interest of getting this fixed sooner. I will look for other issues to try and tackle. |
|
@owocki are claims mutually exclusive, or can multiple people register a claim to the bounty at once? If only one claim can be registered at a time, I think it's best if we reject @kammerdiener's claim until he's resubmitted. I really appreciate your work, @kammerdiener, but I worry that people may see a claim for the bounty here and be put off from trying to solve the issue, when the truth is that the field is still wide open. |
|
@clehene are you still intending to complete this fix? I'd love to award this bounty if possible. Either way, @seesemichaelj is going to take over responsibility from me for tracking this. I'll still be around, however if you guys have any questions for me. |
|
@benjamincburns @seesemichaelj this got more or less blocked with the final bits as I saw two different ways which depend on input from EVM / merkle-tree. I.e. we'd need to understand the author's intentions there as the logic is a bit convoluted and undocumented or otherwise discuss what's the best thing based on context. I wouldn't mind taking another stab. TBH the bounty was just the cherry on top, I mostly wanted to get my feet wet and have it done, but I need to discuss with someone. I like to finish things when I start them, but getting the whole context back and rebasing it will take some effort. I need to go over notes, etc. as I don't fully remember all bits - so I may need to spend some time and would only do that if I'm sure I can get proper support and not sure whom to ask for that :) |
|
Hey @clehene; sorry I'm just getting around to this now. Please mind me as I'm getting up to speed what the issue is and possible theories/solutions. As afar as getting input from EVM/merkle-tree, I think one of the best people to start a discussion will be @holgerd77. However, just looking at the history of Looking back on the stuff you said March 30, it may be best to go ahead and submit PRs and probe questions. It'd help if you have somewhat direct questions as well as a detailed description of your proposed changes. Are the commits clehene/merkle-patricia-tree@4927c76 and clehene/merkle-patricia-tree@25541d4 potential solutions to the problem @clehene? |
|
@holgerd77 please see @clehene's last comment. Are you the right person to help us navigate getting this change accepted? If you're not familiar w/ this issue, I'd be happy to set up a call with you to discuss. Would love it if @clehene joined as well. Either way, we haven't heard from @clehene in a while and the price of ETH is drastically different from when we first opened this bounty. As a result we're going to adjust the funding and clear out @clenehe's currently active claim. That said, if @clehene is still interested I'd be thrilled if he picked it back up - he's definitely carried it most of the way. |
|
Hey @seesemichaelj didn't intend to go AWOL.
As much as I'd love to finish this, I'm not sure it'd be realistic just based on how much time I estimate it would take. I was not able to reproduce all faulty conditions last time I rebased things (that was after June 13th). Before that I remember going rather deep in the VM last time I had a clue of what could be the root cause and expose the effect we were hunting for. For that I was debugging and stepping through VM and instrumented the VM along with libraries to get detailed tracing information. That makes rebasing costly as it's essentially carrying tracing parameters across the entire call stack. So long story short it's a full time job for 1-2 weeks to get up to speed :). Anyways, if there's indeed an active claim, I'm sorry for that and hope it didn't deter anyone from taking a stab at it :). A final thought. I think it may be worth considering rewriting the merkle tree implementation from scratch by someone that has a clear idea of how it needs to behave, etc. (I remember going through the yellow paper that there were some peculiarities there too). I honestly think that would be the best and cheapest long term maintenance solution. |
|
Issue Status: 1. Open 2. Started 3. Submitted 4. Done This issue now has a funding of 400.0 DAI (400.0 USD @ $1.0/DAI) attached to it.
|
|
@benjamincburns Sorry, I have to prioritize other time-critical tasks for now. Can help to get this settled mid-term hopefully (8-12 weeks first estimation). |
|
Update: For example, as demonstrated in the test case, the following events can be problematic:
Proposal: |
|
Issue Status: 1. Open 2. Started 3. Submitted 4. Done Work for 400.0 DAI (400.0 USD @ $1.0/DAI) has been submitted by: @ceresstation please take a look at the submitted work:
|
|
I commented over on the PR, but I figure I should comment here. @kn I really think @clehene identified the root cause already, and the changes he listed on 1 March likely addressed the true underlying issues. The most important problem he identified was that the MPT's |
|
Hey @PaulRBerg, @raboid are either of you interested in giving this a shot? Please also see the reference issue at the bottom. @kn really appreciate your work, going to pay out a tip separate from the bounty for your effort! |
|
Thanks! Appreciate it. Here is what I left off with and changes I made are: The next steps were
|
|
hey @ceresstation how is this bounty proceeding? Looks like @benjamincburns closed a PR that kn started due to some extra complexity, but is it safe to say the bounty is still open? |
|
@frankchen07 Yes, this bounty is still open |
|
Closing this one, as it was fixed by trufflesuite/ganache#209 We didn't fund this bounty ourselves, so I'm not sure how to go about compensating those who helped out. I think @kn and @clehene should split the reward, as their solutions were both viable and they ultimately lead me to the solution in the above PR. I'll check in w/ the GitCoin folks next week and see what we can do. |
|
@benjamincburns - gitcoin.co/tip is a way to compensate those who helped close out an issue (but not necessarily the ones who submitted the work). |
|
@benjamincburns nice! was it fixed entirely through trufflesuite/ganache#209 or did it require changes in other parts? |
|
Fully agree @benjamincburns, I'll split the reward between @kn and @clehene :) |
|
⚡️ A tip worth 200.00000 DAI (200.0 USD @ $1.0/DAI) has been granted to @kn for this issue from @ceresstation. ⚡️ Nice work @kn! Your tip has automatically been deposited in the ETH address we have on file.
|
|
Great work - glad to see it fixed! Thanks for the generous tip. |
|
This bounty should no longer be open.
…On Wed, Feb 6, 2019 at 1:24 AM Lazaridis ***@***.***> wrote:
@ceresstation <https://github.com/ceresstation> , the bounty is still
listed as open on gitcoin.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<trufflesuite/ganache#453 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAxAyMO4Q6XOmK1PpFBiZzTiUSOv4Z46ks5vKpEkgaJpZM4RMaid>
.
|
|
@ceresstation mind closing the bounty? I just removed the 'open' override status, and that made it appear as 'submitted'. If you cancel the bounty that should close it out. |
|
Issue Status: 1. Open 2. Started 3. Submitted 4. Done This Bounty has been completed. Additional Tips for this Bounty:
|
|
⚡️ A tip worth 200.00000 DAI (200.0 USD @ $1.0/DAI) has been granted to @clehene for this issue from @ceresstation. ⚡️ Nice work @clehene! Your tip has automatically been deposited in the ETH address we have on file.
|
As a result of #450, in trufflesuite/ganache#41, @0xNPE gave us some much needed tests which reproduce the underlying problems behind #417. This bounty is for a fix for the underlying problem which those tests identify.
Bounty acceptance criteria:
ganache-coreor any other impacted projects.Important note: This bounty will not be paid out until all required PRs are accepted and merged.
Tips:
ganache-coreyou can usenpm linkto symlink your local copy of the dependency into thenode_modulesdirectory of theganache-coreproject.standardtool to verify that you have done so.The text was updated successfully, but these errors were encountered: