*: attach full transactions to PrepareRequest - #160
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #160 +/- ##
==========================================
- Coverage 57.65% 54.83% -2.83%
==========================================
Files 33 33
Lines 1913 1882 -31
==========================================
- Hits 1103 1032 -71
- Misses 717 754 +37
- Partials 93 96 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
087c01c to
945ab02
Compare
945ab02 to
514cc36
Compare
Signed-off-by: Tural Devrishev <tural@nspcc.ru>
514cc36 to
8ecec1c
Compare
AnnaShaleva
left a comment
AnnaShaleva
left a comment
There was a problem hiding this comment.
Let's align dbft code wrt nspcc-dev/neo-go@ff4eb10. No decoding optimisation is required. Both old (master) and new (feature) behaviour should be preserved, the behaviour switch (something like a configurable callback or an additional parameter to (DBFT).Start or (DBFT).Reset) should be exposed to the external user to enable the extension starting from some point without dBFT shutdown.
| dbft.WithCurrentBlockHash[crypto.Uint256](currentBlockHash), | ||
| dbft.WithGetValidators[crypto.Uint256](getValidators), | ||
| dbft.WithVerifyPrepareRequest[crypto.Uint256](verifyPayload), | ||
| dbft.WithUnpackTransactions[crypto.Uint256](UnpackTransactions), |
There was a problem hiding this comment.
Since we decided to drop the decoding optimisation, let's remove this callback.
| for i, tx := range ctx.TransactionsOrdered { | ||
| txHashes[i] = tx.Hash() | ||
| } | ||
| block := NewBlock(ctx.Timestamp, ctx.BlockIndex, ctx.PrevHash, ctx.Nonce, txHashes) |
There was a problem hiding this comment.
We need a context-level switch (something like boolean field will suit). Depending on this switch we'll use either hashes (the old behaviour) or transactions (the new behaviour) to construct prepare request (and block). This switch should be activated on the caller's side (i.e. by NeoGo at Huyao hardfork) at the runtime without consensus service interruption.
| // Transactions is a map containing actual transactions for the current block. | ||
| Transactions map[H]Transaction[H] | ||
|
|
||
| TransactionsOrdered []Transaction[H] |
There was a problem hiding this comment.
Here we should maintain both TransactionHashes and Transactions fields. The first one should be used for pre-Huyao behaviour, the second one should be used for post-Huyao behaviour.
The similar logic should be applied to other places where the old code is replaced by the new approach.
| d.sendPrepareResponse() | ||
| d.checkPrepare() | ||
| } | ||
| panic("dbft: addTransaction must not be called, PrepareRequest transactions are always attached in full") |
There was a problem hiding this comment.
Ditto, panic only if the behaviour switch is on. Otherwise the old behaviour should be kept.
2 participants
Extend
PrepareRequestwithTransactions().NewPrepareRequestnow also receives the full transaction bodies, andprocessMissingTxtakes them as an argument, so a backup can pull a missing transaction straight from the request instead of a network request that may go unanswered. See nspcc-dev/neo-go#4325.