Skip to content

Conversation

@superzordon
Copy link
Contributor

No description provided.

@superzordon superzordon requested a review from a team as a code owner March 25, 2023 22:23
@@ -0,0 +1,188 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this file or was this just for testing?

// Create a new view object.
utxoView, err := NewUtxoView(desoBlockProducer.chain.db, desoBlockProducer.params,
desoBlockProducer.postgres, desoBlockProducer.chain.snapshot)
desoBlockProducer.postgres, desoBlockProducer.chain.snapshot, nil)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a rule-of-thumb for when we need to include the event manager or not?

Copy link
Member

Choose a reason for hiding this comment

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

I think we always include it except for weird edge-cases. In this case, getBlockTemplate runs outside of the main event loop, and just generates blocks for miners. I would expect mempool may be the one other place where we don't include the EventManager.

TxHash: txHash,
IsConnected: true,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we have some emit logic at the transaction level? If I just do a basic transfer, do we get all the transaction fields passed through somewhere? Wondering if we can get all the outputs in the new state syncer db, so we can see "oh you sent money to x, y, and z"

})

if bav.EventManager != nil && emitMempoolTxn {
bav.EventManager.mempoolTransactionConnected(&MempoolTransactionEvent{
Copy link
Member

Choose a reason for hiding this comment

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

do we want to emit state changes for balance changes when the creator gets a founder reward?

return bav.FlushToDbWithTxn(txn, blockHeight)
})
if bav.EventManager != nil {
bav.EventManager.dbFlushed(&DBFlushedEvent{
Copy link
Member

Choose a reason for hiding this comment

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

Need to look into what this does

Comment on lines 256 to 268
// Get the cached mempool transaction from the connected mempool map.
if connectedMempoolTx, ok := stateChangeSyncer.ConnectedMempoolTxMap[*event.TxHash]; ok {
// Check to see if the index in question has a "core_state" annotation in its definition.
if !isCoreStateKey(connectedMempoolTx.KeyBytes) {
return
}

stateChangeEntry.Encoder = connectedMempoolTx.Encoder
stateChangeEntry.KeyBytes = connectedMempoolTx.KeyBytes
stateChangeEntry.UtxoOps = connectedMempoolTx.UtxoOps
} else {
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Get the cached mempool transaction from the connected mempool map.
if connectedMempoolTx, ok := stateChangeSyncer.ConnectedMempoolTxMap[*event.TxHash]; ok {
// Check to see if the index in question has a "core_state" annotation in its definition.
if !isCoreStateKey(connectedMempoolTx.KeyBytes) {
return
}
stateChangeEntry.Encoder = connectedMempoolTx.Encoder
stateChangeEntry.KeyBytes = connectedMempoolTx.KeyBytes
stateChangeEntry.UtxoOps = connectedMempoolTx.UtxoOps
} else {
return
}
// Get the cached mempool transaction from the connected mempool map.
connectedMempoolTx, ok := stateChangeSyncer.ConnectedMempoolTxMap[*event.TxHash]
if !ok {
return
}
// Check to see if the index in question has a "core_state" annotation in its definition.
if !isCoreStateKey(connectedMempoolTx.KeyBytes) {
return
}
stateChangeEntry.Encoder = connectedMempoolTx.Encoder
stateChangeEntry.KeyBytes = connectedMempoolTx.KeyBytes
stateChangeEntry.UtxoOps = connectedMempoolTx.UtxoOps

Copy link
Member

Choose a reason for hiding this comment

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

early return and avoiding else statements where possible.

if isEncoder, encoder := StateKeyToDeSoEncoder(stateChangeEntry.KeyBytes); isEncoder && encoder != nil {
encoderType = encoder.GetEncoderType()
} else {
// If the keyBytes is not an encoder, then we decode the entry from the key value.
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment that this is expected for things such as LikeEntry where the entry is decoded from the key. the comment is a little off at the end - the phrase "key value" is a little confusing.


// Encode the state change entry. We encode as a byte array, so the consumer can buffer just the bytes needed
// to decode this entry when reading from file.
entryBytes := EncodeToBytes(0, stateChangeEntry, false)
Copy link
Member

Choose a reason for hiding this comment

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

hmmm we probably need a real block height in this call to EncodeToBytes, right? This gets passed down into RawEncodeWithoutMetadata

writeBytes := EncodeByteArray(entryBytes)

decodedStateChangeEntry := &StateChangeEntry{}
DecodeFromBytes(decodedStateChangeEntry, bytes.NewReader(entryBytes))
Copy link
Member

Choose a reason for hiding this comment

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

do we want to catch the error from DecodeFromBytes?


// Update the state change file size.
transactionLen := uint32(len(writeBytes))
unflushedBytes.StateChangeBytesSize += transactionLen
Copy link
Member

Choose a reason for hiding this comment

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

maybe add overflow check? We have these SafeUint utils lying around

Copy link
Member

Choose a reason for hiding this comment

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

+1 (assuming we keep this field-- see my other comment)

}

// Get the relevant deso encoder for this keyBytes.
var encoderType EncoderType
Copy link
Member

Choose a reason for hiding this comment

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

More comments here. I want to better understand when something hits the first case vs the second. I think it happens whenever we have a field in the db that's a "key only" field with no Entry as a value.

Comment on lines 593 to 595
err = mempoolUtxoView.FlushToDbWithTxn(txn, uint64(server.blockchain.bestChain[len(server.blockchain.bestChain)-1].Height))

if err != nil || originalCommittedFlushId != stateChangeSyncer.BlockSyncFlushId {
Copy link
Member

Choose a reason for hiding this comment

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

The second part of this is tricky. I think it's checking whether a block came in while we were mempooling. Regardless, add comment.

Comment on lines 40 to 42
AncestralRecord DeSoEncoder
// The ancestral record represented in bytes.
AncestralRecordBytes []byte
Copy link
Member

Choose a reason for hiding this comment

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

Add comment on when ancestral record bytes is used. I assume in hypersync it's not pupulated for example. Also I think it's only for upsert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's only for upsert. We only use ancestral records when syncing mempool entries. When the new block syncs, we need to revert any mempool entries we've stored in the database before applying the committed entries. We do this reversion by reverting to the ancestral record value.

StateChangeBytes []byte
StateChangeBytesSize uint64
// This is a list of the indexes of the state change bytes that should be written to the state change index file.
StateChangeOperationIndexes []uint64
Copy link
Member

Choose a reason for hiding this comment

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

Give comments on these fields that explains how they work.


// During blocksync, we flush all entries by index to the state change file once the sync is complete.
// This is used to track whether this procedure has been initiated.
BlocksyncCompleteEntriesFlushed bool
Copy link
Member

Choose a reason for hiding this comment

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

Explain the deeper reasons for why we do this optimization, rather than just flushing with each block. We want to give a lot of context for optimizations like these so that we can revisit them intelligently in the future. Otherwise, people are scared to change things like this for fear it will break things in unexpected ways.

cmd/node.go Outdated
// Setup chain database
dbDir := lib.GetBadgerDbPath(node.Config.DataDirectory)
opts := lib.PerformanceBadgerOptions(dbDir)
opts := badger.DefaultOptions(dbDir)
Copy link
Member

@diamondhands0 diamondhands0 Aug 3, 2023

Choose a reason for hiding this comment

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

Revert to PerformanceBadgerOptions per our conversation

// Initialize the ancestral records database
snapshotDirectory := filepath.Join(GetBadgerDbPath(mainDbDirectory), "snapshot")
snapshotOpts := PerformanceBadgerOptions(snapshotDirectory)
snapshotOpts := DefaultBadgerOptions(snapshotDirectory)
Copy link
Member

Choose a reason for hiding this comment

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

Revert to PerformanceBadgerOptions per our conversation

--add-ips=localhost:19000 \
--testnet=true \
--api-port=18001 \
--protocol-port=18000 \
Copy link
Member

Choose a reason for hiding this comment

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

Can we delete the whole nodes folder here? People should only use the ones in backend from now on. These are outdated and misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense to me 👍

_, _, _, _, err = utxoViewCopy._connectTransaction(
mempoolTx.Tx, mempoolTx.Hash, int64(mempoolTx.TxSizeBytes), uint32(blockRet.Header.Height), true,
false /*ignoreUtxos*/)
_, _, _, _, err = utxoViewCopy._connectTransaction(mempoolTx.Tx, mempoolTx.Hash, int64(mempoolTx.TxSizeBytes), uint32(blockRet.Header.Height), true, false)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if you could do a pass to fix all the line lengths that the refactoring broke. With vim you can highlight a line and hit "gq" and it does it. But not sure what the shortcut is in other things.

if innerErr = bc.blockView.FlushToDbWithTxn(txn, blockHeight); innerErr != nil {
return errors.Wrapf(innerErr, "ProcessBlock: Problem writing utxo view to db on simple add to tip")
}
if bc.eventManager != nil && !bc.eventManager.isMempoolManager {
Copy link
Member

Choose a reason for hiding this comment

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

Do me a favor and just add a comment explaining why we put this event trigger here rather than elsewhere. For example, is there a reason why we need to put it in the badger Update() vs outside of it? Just explain it, or explain that the placement doesn't matter, in case someone needs to move things around in the future.

EncoderBytes: dbEntry.Value,
}

fmt.Printf("Flushing entry %d to file\n", ii)
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to leave all these prints in here? Fine if they're intentional I'm just flagging it.

@superzordon superzordon merged commit c79a756 into deso-protocol:main Nov 9, 2023
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.

3 participants