use pool cookie time instead of wall time #5
Conversation
- `/getblocks.bin` respects the `RESTRICTED_TX_COUNT` (=100) when
returning pool txs via a restricted RPC daemon.
- A restricted RPC daemon includes a max of `RESTRICTED_TX_COUNT` txs
in the `added_pool_txs` field, and returns any remaining pool hashes
in the `remaining_added_pool_txids` field. The client then requests
the remaining txs via `/gettransactions` in chunks.
- `/gettransactions` no longer does expensive no-ops for ALL pool txs
if the client requests a subset of pool txs. Instead it searches for
the txs the client explicitly requests.
- Reset `m_pool_info_query_time` when a user:
(1) rescans the chain (so the wallet re-requests the whole pool)
(2) changes the daemon their wallets points to (a new daemon would
have a different view of the pool)
- `/getblocks.bin` respects the `req.prune` field when returning
pool txs.
- Pool extension fields in response to `/getblocks.bin` are optional
with default 0'd values.
Using `time(NULL)` to order incoming transactions has problems because the following could happen: 1. Wallet asks for current daemon time and daemon responds with time A 2. Daemon system time resynchronizes and falls backwards in time to time B (B < A) 3. Daemon add transaction T to mempool at time C (B < C < A) 4. Wallet asks for incremental changes since time A, daemon responds 5. Since C < A, transaction T is not sent to wallet 6. Wallet refresh is now in inconsistent state
There was a problem hiding this comment.
Thanks for this PR, interesting approach. I went through the code, I think I understand it, and I think it does what you intended.
However I still claim that with the system as I implemented it, after the daemon suffers a local time change, things self-correct fast, as each connected client will only miss new pool transactions exactly once and will see those missed transactions anyway with the next block.
And with this in mind, I am not sure whether the trade-offs are favorable - rare event of daemon local time change versus added complexity to read and and write that "logical timestamp" in the DB.
Plus, if the daemon crashes, your system may result in problems, where my time-base system doesn't, because you won't be able anymore to update the DB with the new value, as @moneromooo mentioned on IRC. Tough question whether local time changes or daemon crashes are more frequent ...
But anyway, if @j-berman views this favorably, I am ready to merge.
| if (m_removed_txs_by_time.size() > MAX_REMOVED) | ||
| { | ||
| auto erase_it = m_removed_txs_by_time.begin(); | ||
| decltype(m_removed_txs_by_time)::iterator erase_it = m_removed_txs_by_time.begin(); | ||
| std::advance(erase_it, MAX_REMOVED / 4 + 1); | ||
| m_removed_txs_by_time.erase(m_removed_txs_by_time.begin(), erase_it); | ||
| m_removed_txs_start_time = m_removed_txs_by_time.begin()->first; | ||
| MDEBUG("Erased old transactions from big removed list, leaving " << m_removed_txs_by_time.size()); | ||
| } |
There was a problem hiding this comment.
A bit unfortunate that we can't delete anymore within a certain time period. If you had an early burst of transactions and then a lull of a few hours you will delete pretty far back.
| @@ -1957,7 +1947,7 @@ namespace cryptonote | |||
| } | |||
|
|
|||
| m_mine_stem_txes = mine_stem_txes; | |||
| m_cookie = 0; | |||
| m_cookie = m_blockchain.get_txpool_logical_timestamp(); | |||
|
|
|||
There was a problem hiding this comment.
Was that your question on IRC, whether you can use that m_blockchain object already at the time of this init method here? I guess the answer was a sure "yes".
|
As mentioned, the following still looks possible:
If I'm reading it right, I appreciate that this code only reads from/writes to the db on startup/close because another db read/write wouldn't be ideal in the pool update code, but as a result there is the issue of potential inconsistent state on crash. This could be improved by saving the logical timestamp in the mempool's As mentioned in no-wallet-left-behind, either the daemon could start responding with a session ID and the client can handle that, or if the client loses its connection to the daemon it could be sure to re-request all pool txs from that node by resetting On an immediate path forward, I think @rbrunner7's point here is especially valid:
Because of this, I think we should try to move forward with monero-project#8076 as is, and can perfect it in a later PR. |
Technically, you would only need to save values which users have seen, so you only need to save when the value was updated + fetched to save some reads/writes. But at any rate, I guess I'm fine with keeping the timing as-is on the condition that the wallet is time source agnostic. |
Not sure how you mean that, @jeffro256. You mean you see actually using the "real" daemon tool as an info leak that we have to solve still within the frame of this PR? |
|
No, just if future steady clock replacements on the daemon side are backwards compatible with wallets on this PR. |
multisig: fix segfault restoring encrypted multisig seed
12e7c41 Merge pull request #5 from j-berman/restore-msig-encrypted-seed (Justin Berman) 848a0c0 Fix segfault restoring encrypted multisig seed (j-berman) 401f5d9 Require user ack multisig is experimental to restore (j-berman) fc8a5d6 multisig: fix monero-project#8537 seed restore (suggestions by @UkoeHB) (j-berman)
…p_speedup src: update internal data structure to boost::bimap.
Using
time(NULL)to order incoming transactions has problems because the following could happen: