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

Fix data handling if dao is deactivated #5711

Merged

Conversation

chimp1984
Copy link
Contributor

@chimp1984 chimp1984 commented Sep 20, 2021

Adds a toggle for deactivating the DAO and fixes an issue with P2P data processing if DAO is deactivated.

are still received from seed nodes and processed but
as the services for processing the payloads are not
added the data is inefficiently processed.
The getMap returned a flattened map of all maps in
all services which can be quite large.
We use now a filtered map with calling canHandle
first. Also the put got optimized to indicate in the
return value if there has been a service found to add
the payload. If not we do not invoke the listeners and
do not broadcast.

To not request the DAO P2P data would be better but I
don't see a easy way how to do that as the P2P network
is not aware of the type of data. Some market interface
could be used and a flag at the request to the seed node
to indicate if those types should be included but that
does feel too customized for a special use case. The
DAO P2P data is not that big as well, so I think for now
that fix should be good enough.
We write the value to the properties file not to
the preferences as we need the value early at startup
before the preferences are loaded.
Use appendOnlyDataStoreService.getMap(persistableNetworkPayload)
instead.
Make it more clear that we expect only one
matching service which can handle the payload.
@chimp1984
Copy link
Contributor Author

Adjusted the DAO view in case DAO is deactivated.

@m52go Could you have a look to the text?

dao.news.daoInfo.description=To participate in the Bisq DAO and to use BSQ for discounted trading fees you need to \
  enable the DAO. If the DAO is enabled, it downloads all the missing blocks and verifies the BSQ transactions. \
  This can take some time and consumes 100% of the available resources during the verification process.

DAO view in case DAO is deactivated:
Screen Shot 2021-09-23 at 19 00 58

Settings when DAO is deactivated. Shows activate toggle.
Screen Shot 2021-09-23 at 19 00 49

Settings when DAO is activated.
Screen Shot 2021-09-23 at 19 01 42

@m52go
Copy link
Contributor

m52go commented Sep 23, 2021

Suggestion:

dao.news.daoInfo.description=To participate in the Bisq DAO and to use BSQ for discounted trading fees, you need to \
  enable the DAO. When the DAO is enabled, Bisq downloads all missing blocks and verifies BSQ transactions. \
  This verification process requires time, during which you may see Bisq use a lot of memory and processing power. This is normal.

@ghubstan
Copy link
Contributor

Tested ACK

Tested DAO view is adjusted for daoActivated true or false. OK

Tested bisq.properties editing (daoActivated=true/false). OK

Tested --daoActivated program argument is set it has higher priority and property file value is ignored. OK

Tested dao activation/deactivation from UI. OK

Tested appendOnlyDataStoreListeners called only after successful AppendOnlyDataStoreService.put(hash,payload) into Optional<MapStoreService>. OK

Tested we expect only one matching service which can handle the payload. OK

Ran modified test cases / all passed. OK

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK based on #5711 (comment)

@ripcurlx ripcurlx added this to the v1.7.5 milestone Sep 26, 2021
@ripcurlx ripcurlx merged commit ccb73a3 into bisq-network:master Sep 26, 2021
@chimp1984 chimp1984 deleted the fix-data-handling-if-dao-is-deactivated branch September 28, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants