feat!: improve performance of HandleTransactionGet, by requesting all transactions from InvMsg at once

[kuba-4chain]

What has been done

Method
HandleTransactionGet(msg *wire.InvVect, peer PeerI) ([]byte, error)
was changed to
HandleTransactionsGet(msg []*wire.InvVect, peer PeerI) ([][]byte, error).

This change allows for more efficient transaction getting, because it bundles all tx requests from the InvVect message, instead of asking for transactions one-by-one.

Why I modified this method, instead of leaving it as it was and adding another one?

1. The method was used only in one place, therefore adding another method would make the first one unused.
2. It doesn't change the logic of sending transactions to the peer, it just changes the logic of retrieving transactions from database from a client of go-p2p.

Potential downsides

1. Breaking changes. Current clients of go-p2p need to adapt to this change, but I believe it's a useful and efficient change.
2. Less information about errors for particular transactions. In previous approach, an error from getting each transaction was logged, and another transaction was requested. In new approach, if an error occurs by getting all transactions from InvVect message, no transactions are broadcasted.

[shotasilagadzetaal]

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.

It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.

Am I missing something here ? 🤔

[arkadiuszos4chain]

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.

It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.

Am I missing something here ? 🤔

Not really. With this change we can limit database round trips as we request for few transactions in one call.

[shotasilagadzetaal]

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.
It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.
Am I missing something here ? 🤔

Not really. With this change we can limit database round trips as we request for few transactions in one call.

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.
It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.
Am I missing something here ? 🤔

Not really. With this change we can limit database round trips as we request for few transactions in one call.

Where is the decrease of DB calls in here? What do you mean? I still don't see it

[arkadiuszos4chain]

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.
It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.
Am I missing something here ? 🤔

Not really. With this change we can limit database round trips as we request for few transactions in one call.

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.
It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.
Am I missing something here ? 🤔

Not really. With this change we can limit database round trips as we request for few transactions in one call.

Where is the decrease of DB calls in here? What do you mean? I still don't see it

This is the current implementation of PeerHandlerI in ARC metamorph. As you can see, we currently call the database for every transaction in a separate request.

With the new version, the client (e.g., ARC metamorph) can fetch data about all requested transactions in a single trip to the database.

Sample impl:

So, this change is not a performance improvement per se, but it provides the possibility for improvement by the library client.

[shotasilagadzetaal]

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.
It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.
Am I missing something here ? 🤔

Not really. With this change we can limit database round trips as we request for few transactions in one call.

I still don't understand how the change speeds up the processing. As I see, you simply moved for loop from one function to another.
It doesn't call the function for each hash but passes those hashes together as a slice. But the work in the next function is the same - you go through all the hashes/txs and do the same operation. So the only change here is the place of the for loop.
Am I missing something here ? 🤔

Not really. With this change we can limit database round trips as we request for few transactions in one call.

Where is the decrease of DB calls in here? What do you mean? I still don't see it

This is the current implementation of PeerHandlerI in ARC metamorph. As you can see, we currently call the database for every transaction in a separate request.

With the new version, the client (e.g., ARC metamorph) can fetch data about all requested transactions in a single trip to the database.

Sample impl:

So, this change is not a performance improvement per se, but it provides the possibility for improvement by the library client.

I see, I was just looking for optimisation in p2p code. That makes sense now. Please consider backwards compatibility. I think it's kinda hard fork with this change.