disable HandleTransactionAnnouncement #529
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
shotasilagadzetaal
force-pushed
the
disable-handle-transaction-announce
branch
from
da9e1b1
to
f2f11cd
Compare
|
| @@ -45,15 +45,6 @@ func (m *PeerHandler) HandleTransactionSent(msg *wire.MsgTx, peer p2p.PeerI) err | |||
|
|
|||
| // HandleTransactionAnnouncement is a message sent to the PeerHandler when a transaction INV message is received from a peer. | |||
| func (m *PeerHandler) HandleTransactionAnnouncement(msg *wire.InvVect, peer p2p.PeerI) error { | |||
| select { | |||
There was a problem hiding this comment.
Please add comment why we do not handle INV messages
| } | ||
|
|
||
| type AnnouncedTransaction struct { | ||
| second uint64 |
There was a problem hiding this comment.
| second uint64 | |
| timestamp uint64 |
There was a problem hiding this comment.
It's not actually timestamp, it's simply number of seconds.
There was a problem hiding this comment.
It's the number of seconds since the Unix epoch. It is used to determine if an object is older than a specific moment in time. Seems like a timestamp to me
internal/metamorph/processor.go
Outdated
| case <-ticker.C: | ||
| p.announcedTransactionsLock.Lock() | ||
| for k := 0; k < len(p.announcedTransactions); k++ { | ||
| if p.announcedTransactions[k].second < uint64(time.Now().Unix())-1 { |
There was a problem hiding this comment.
- use
p.now() - move calculating timestamp (
uint64(time.Now().Unix())-1) above the loop - remove redundant casting from
p.pm.RequestTransaction((*chainhash.Hash)(p.announcedTransactions[k].hash))
There was a problem hiding this comment.
Don't understand second point here.
There was a problem hiding this comment.
for k := 0; k < len(p.announcedTransactions); k++ {
if p.announcedTransactions[k].second < uint64(time.Now().Unix())-1{
(...)
}
(...)
}
To:
until := uint64(p.Now().Unix())-1
for k := 0; k < len(p.announcedTransactions); k++ {
if p.announcedTransactions[k].second < until{
(...)
}
(...)
}
| break | ||
| } | ||
| } | ||
| p.announcedTransactionsLock.Unlock() |
There was a problem hiding this comment.
Reset the ticker at the end
| @@ -689,6 +727,9 @@ func (p *Processor) ProcessTransaction(ctx context.Context, req *ProcessorReques | |||
| return | |||
| } | |||
|
|
|||
| // will be requesting transaction after ~2 seconds to get SEEN_ON_NETWORK status | |||
There was a problem hiding this comment.
This operation may be blocking. Can we move it to the end of the processing? Does it make sense?
There was a problem hiding this comment.
What do you mean blocking? It's simple mutex lock nothing complicated here.
There was a problem hiding this comment.
Yes, but we lock the same collection in StartCheckingTransactionsInNetwork
There was a problem hiding this comment.
Sorry still confused. Yes we lock it in StartCheckingTransactionsInNetwork too but why is it a problem?
There was a problem hiding this comment.
Let's assume we have heavy traffic and receive a lot of transactions.
StartCheckingTransactionsInNetworklocks theannouncedTransactioncollection and starts sending messages to the node.- Many transactions are sent to ARC and announced to the network.
- The transactions are waiting for the announcedTransaction collection to be released.
In this case, isn't it a problem that we are delaying the processing of transactions (status updates, responses to the client) until the transactions can be added to the collection?
| @@ -384,6 +394,37 @@ func (p *Processor) StartSendStatusUpdate() { | |||
| }() | |||
| } | |||
|
|
|||
| func (p *Processor) StartCheckingTransactionsInNetwork() { | |||
| p.waitGroup.Add(1) | |||
| ticker := time.NewTicker(100 * time.Millisecond) | |||
There was a problem hiding this comment.
Open question: Is it necessary to lock the announcedTransactions collection every 100 milliseconds? A better approach might be to increase the interval to 0.5-1 second and decrease the delta between the current time and the transaction announcement time. WDYT?
There was a problem hiding this comment.
Honestly not sure what we need here exactly. Or in other words what's the best time interval to wait before requesting transaction.
|
Closed because the fix for issue of too much DB load will be resolved later |
We have memory issue caused by processing too many messages in HandleTransactionAnnouncement. On every message we create new go routine which pushes message into the channel. When checking the leak cause I see too many go rougines blocked on the channel push. Having many go rougines created caused memory issue.
We don't actually need to process every transaction with this message. We will get SEEN_ON_NETWORK status for our transactions from HandleTransaction handler instead.