<p><b>@ManfredKarrer</b> commented on this pull request.</p>

<p>I would suggest renaming <code>toProtoMessageSynchronized</code> to <code>toPersistableMessage</code>.<br>
Also would suggest to make it more explicit and flexible by adding the new behaviour in a <code>UserThreadMappedPersistableEnvelope</code> interface and keep PersistableEnvelope as non-safe-thread but fast interface with the default method:<br>
<code>default Message toPersistableMessage() { return toProtoMessage(); }</code></p>
<p>If this is applied it has to be carefully checked for each persisted object which interface is most suitable and check with profiling how the changes compare. There are some generic classes like <code>PersistableList</code> which need to be considered as well (at the moment its all covered as <code>PersistableEnvelope</code> is used for the new method.</p>
<p>There are several classes implementing <code>PersistableEnvelope</code> like <code>SignedWitness</code> but I think that is wrong as they are not persisted directly but wrapped in a storage class. I have not checked more closely but might be worth to clean that up in a separate PR.</p>
<p>I am not sure about the performance impacts when using UserThread mapping. Any delay > 20 ms is visually visible in the UI as lagging.</p>
<p>I would suggest to create 3 categories for all persisted objects:</p>
<ul>
<li>Big objects -> performance is critical, use threaded version (DaoState,...)</li>
<li>Small objects, correctness is critical -> use UserThread mapping (SequenceNrMap,...)</li>
<li>Other uncritical -> use non-thread safe option in dedicated thread (NavigationPath)</li>
</ul>
<p>The uncritical ones could be kept as non-threadsafe IMO.</p>
<p>For further improvements the delays between write operations should be checked, there might be some optimisation possible, specially for those which have frequent write operations.</p>
<p>From the DAO perspective I don't see any obvious risk with that change, but it should be tested very well specially with DaoStateHashes and the monitoring for the 3 different hash chains.</p>
<p>For the most problematic objects (DaoState, TradeStatistics, AccountAgeWitness) I suggest to start to look into a different design as suggested in the related issues/PRs (e.g. use a historical data structure which is not handled by the P2PDataStorage and only loaded in memory on demand). For DaoState that approach might not work, but some solution to make it more scalable is required here as well.</p><hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/4025#discussion_r389422679">common/src/main/java/bisq/common/proto/persistable/PersistableEnvelope.java</a>:</p>
<pre style='color:#555'>>  
 /**
  * Interface for the outside envelope object persisted to disk.
  */
 public interface PersistableEnvelope extends Envelope {
+
+    default Message toProtoMessageSynchronized() {
</pre>
<p>I find the method name a bit confusing as it is only synchronized in <code>ThreadedPersistableEnvelope</code> but here we use mapping to userThread. I would recommend using <code>toPersistableMessage</code> as stated above.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/4025#discussion_r389422956">common/src/main/java/bisq/common/proto/persistable/PersistableEnvelope.java</a>:</p>
<pre style='color:#555'>>  
 /**
  * Interface for the outside envelope object persisted to disk.
  */
 public interface PersistableEnvelope extends Envelope {
+
</pre>
<p>Maybe its better to use a specific interface here as well like in <code>ThreadedPersistableEnvelope</code>? Might make it more explicit that we alter the basic non-thread-safe but fast behaviour. We could use then <code>toPersistableMessage</code> as method name for all 3 cases and the interface marks which style is actually used. Would make it also a bit more distinct from the generic toProtoMessage method and shows its only relevant for persisting an envelope.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/4025?email_source=notifications&email_token=AJFFTNSWYN7M5Y6P373BTPTRGRCWRA5CNFSM4LBEPX62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYNQYAA#pullrequestreview-370871296">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNWH3LL4ZGV6GIL6QFTRGRCWRANCNFSM4LBEPX6Q">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNSDGVJXQ4TJXFHR6O3RGRCWRA5CNFSM4LBEPX62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYNQYAA.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/4025?email_source=notifications\u0026email_token=AJFFTNSWYN7M5Y6P373BTPTRGRCWRA5CNFSM4LBEPX62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYNQYAA#pullrequestreview-370871296",
"url": "https://github.com/bisq-network/bisq/pull/4025?email_source=notifications\u0026email_token=AJFFTNSWYN7M5Y6P373BTPTRGRCWRA5CNFSM4LBEPX62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYNQYAA#pullrequestreview-370871296",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>