[bisq-network/bisq] Add transient tx map to DaoState to speed up getTx queries (#3773)

Julian Knutsen notifications at github.com
Wed Dec 11 21:41:46 UTC 2019


> @julianknutsen
> 
> > The reality is that this code will need to change if we want performance gains. I think it would be a good idea to get everyone on the same page with what can/can't be done in this area and how much testing needs to be a prerequiste to changes.
> 
> Thanks for bringing up the broader discussion!
> 
> My view is that the DAO code should be only touched if there is a very clear reason. Bugs justify the risk to change the code and critical performance issues. There are some layer in the DAO code which is less critical like requesting the BSQ blocks. But the core, the parsing, DaoState and data structures are highly critical consensus code and have to been taken with lot of care.

This is why I think in the future it would be good to show the performance graphs in the PR description as a motivation for the changes. Backing up the code change with the performance data can help the reviewers and maintainers justify the risk involved and quickly decide if it is worth pursuing a particular path. In this case, you had to download and run the code to generate the performance gain which isn't very scalable if you or any reviewers have to do that for every change. Obviously Steven saw the perf gain so I am just suggesting to make it more explicit in the PR description moving forward.

> 
> Regarding testing:
> Of course making the code more testable and adding tests would be great. If that will require non purely technical refactorings (low risk changes) I think we have to discuss the added value and the risk as well to check our overall priorities.

I'm not suggesting that every change in the DAO or otherwise needs to refactor and 100% coverage test the code before anything can be done. I'm only suggesting that an explicit plan for how, when, and who tests this code up-front and as part of the PR description will reduce risk and give explicit responsibility so the quality bar can raise over time.

> 
> For the current state testing can be done by creating all types of DAO transactions and use all DAO features (burn BSQ, voting,...) and create a few cycles with param changes (also radical changes - there we found and fixed a bug which only occurred at more radical changes). The DaoState must be then the same from the original version and the changed version. To resync from genesis is required and running as full DAO node is recommended as well. I usually run 1 full node and one lite node to cover both paths. For dev testing regtest is enough but for the final test this has to be done with the mainnet. A resync with the lite node causes a bit of stress for the live seed nodes, so it should be done only if needed. They only deliver max. 6000 blocks, so it will require repeated BSQ block requests. Also it takes quite a bit of time, but comparing the hashes of the last block gives a the best assurance we have atm regarding the DAO.

Was any of this testing done so far? In the future, I would like to see this type of text as a separate section in the PR written by the author. As we try and move more of the work off of the maintainers and reviewers, it is important that PR authors can show the testing that is done and have a plan for how this will be tested in the next release as well as long-term. This particular code change is relatively benign, but I think it is just the tip of the iceberg in terms of what will need to be done and an effort to be extremely thoughtful about the motivation, tradeoffs, risks, existing testing, new testing, etc is only going to raise the quality bar for the project long-term.

> 
> By the way: As in the past we had problems with merged PRs to the dao domain which have been problematic, we decided that the code inside the dao package in the core module requires an explicit ACK from @ManfredKarrer (see [bisq-network/proposals#119](https://github.com/bisq-network/proposals/issues/119)).
In this particular case, it isn't clear who is in charge of moving this forward. @stejbac has received a lot of feedback and made some changes, but we need a "decider" to help narrow down the lists of "could do" to "must do" so Steven isn't spinning his wheels.



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3773#issuecomment-564745611
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191211/99a3ceeb/attachment-0001.html>


More information about the bisq-github mailing list