Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add RPC send ID for idempotency #610

Merged
merged 1 commit into from
Feb 14, 2018
Merged

Add RPC send ID for idempotency #610

merged 1 commit into from
Feb 14, 2018

Conversation

PlasmaPower
Copy link
Contributor

Probably needs some more discussion.

Note that currently, send IDs are per wallet. If we decide to keep that behavior, it should be documented.

cryptocode
cryptocode reviewed Feb 12, 2018
View reviewed changes
rai/node/wallet.cpp Outdated
ids_path << path_a << "_send_action_ids";
auto error (0);
error |= mdb_dbi_open (transaction_a, path_a.c_str (), MDB_CREATE, &handle);
error |= mdb_dbi_open (transaction_a, ids_path.str ().c_str (), MDB_CREATE, &send_action_ids);
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this pattern is used elsewhere, but this will try to open the second db even if the first one failed. Maybe we should change the pattern to error = error || mdb_dbi_open ... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's much harm to opening the second DB, and it's very unlikely to fail. I'd like to stick with precedent here.

rai/node/wallet.cpp Outdated
block->serialize (stream);
}
auto status (mdb_put (transaction, store.send_action_ids, rai::mdb_val (id->size (), const_cast<char*> (id->data ())), rai::mdb_val (vector.size (), vector.data ()), 0));
assert (status == 0);
Copy link
Contributor

@cryptocode cryptocode Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe send_action should propagate errors to the caller? In this case, in release mode, mdb_put errors are swallowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

cryptocode
cryptocode reviewed Feb 12, 2018
View reviewed changes
rai/node/wallet.hpp Outdated
@@ -5,6 +5,7 @@
#include <rai/secure.hpp>

#include <mutex>
#include <optional>
Copy link
Contributor

Choose a reason for hiding this comment

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

boost/optional.hpp is used elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented Feb 13, 2018 •
edited
Loading

Load tester failed on the last Travis run. This happened before on another recent commit, but it never failed before the hash2 changes. I'm running it locally for some debugging.

lukealonso
lukealonso reviewed Feb 13, 2018
View reviewed changes
rai/node/wallet.cpp Outdated
{
rai::transaction transaction (store.environment, nullptr, false);
if (store.valid_password (transaction))
rai::transaction transaction (store.environment, nullptr, (bool)id_a);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be a write transaction if id_a is set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It updates the send_action_ids store. Should I add a comment?

Copy link
Contributor

@lukealonso lukealonso Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's better to only hold a write transaction for the exact amount of time you need it, since it blocks all other readers. You're not even sure you need it at this point, I would move it the block where you do the mdb_put.

Copy link
Contributor

Choose a reason for hiding this comment

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

(the send queue is serialized, so there's no race)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming it's better to release the old transaction before creating the write transaction, instead of using a nested transaction?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, annoyingly enough.

@PlasmaPower
Copy link
Contributor Author

I'm also planning on returning an error when the wallet::send_action fails, instead of a zero block hash. Thoughts? Is that too much of a breaking change for the RPC?

lukealonso
lukealonso reviewed Feb 13, 2018
View reviewed changes
rai/node/wallet.cpp Outdated
assert (!error2);
block.reset (new rai::send_block (info.head, account_a, balance - amount_a, prv, source_a, generate_work_a ? work_fetch (transaction, source_a, info.head) : 0));
rai::bufferstream stream (reinterpret_cast<uint8_t const *> (i->second.data ()), i->second.size ());
block = rai::deserialize_block (stream, rai::block_type::send);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to store the whole block, or will the just the hash suffice?

If you can't lookup the block by hash to republish it, there's a good chance republishing it won't work anyways since it's probably a fork at that point (the send queue will have moved on, and another block will have taken it's place as the descendant of the parent block at the time).

@PlasmaPower
Copy link
Contributor Author

Fixed @lukealonso's comments, and changed the wallet::send_action failure into an error instead of a zero hash.

@lukealonso
Copy link
Contributor

LGTM - @clemahieu @androm3da This should be merged and released ASAP.

@PlasmaPower
Copy link
Contributor Author

I'd support backporting this change until we're certain the hash2 changes are stable.

@lukealonso
Copy link
Contributor

@PlasmaPower Agreed, this is a critical fix.

@PlasmaPower
Copy link
Contributor Author

Hold off for a bit - this fails to restart.

@PlasmaPower
Copy link
Contributor Author

False alarm - it's some other problem, probably related to the testnet.

@clemahieu
Copy link
Contributor

What do we think about having just one id database that crosses all wallets, would that be simpler? The only difference would be handling on collisions which could be avoided if the caller uses a random number.
Does the test actually prove the idempotency of send? When reusing the same id value?

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented Feb 13, 2018 •
edited
Loading

The test does prove the idempotency with the same id (it asserts the balance and the block returned), and that it'll repeat the action with a different id.

From a code architecture standpoint, I think it makes sense for the database to be per-wallet.

@lukealonso
Copy link
Contributor

It might be better to do it cross-wallet, if only because the number of dbs is limited and doesn't scale well.

@PlasmaPower
Copy link
Contributor Author

That's a good point. One question is concurrency. Are we doing actions for multiple wallets in parallel?

@clemahieu
Copy link
Contributor

It's possible for that to happen, yes, though write transactions are exclusive so we have some concurrency protection there.

We could do both per-wallet and also in a single database by making the key {wallet_id, tx_id} -> hash. I'm not sure if variable keysizes make a performance difference.

@lukealonso
Copy link
Contributor

lukealonso commented Feb 13, 2018 •
edited
Loading

If you resend the same id to a different wallet it should fail, right? (from a user perspective)

@clemahieu
Copy link
Contributor

Honestly I think so. It's very easy to pick a unique, maybe random ID and it seems like it might be more confusing if multiple wallets can have the same id.

@PlasmaPower
Copy link
Contributor Author

So maybe the mapping should be action_id -> {wallet_id, block}?

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented Feb 13, 2018 •
edited
Loading

In which structure should the MDB_dbi be stored if it's global?

@clemahieu
Copy link
Contributor

I'm thinking the rai::wallets class. The reason is at some point i want to separate the ledger from the wallet data and since TX id tracking is related to a wallet operation it should be associated with that.

@PlasmaPower
Copy link
Contributor Author

I think we should either keep the send ids for multiple wallets independent, or return a block for a different wallet if you pass it the same send id. My latest commit does the latter.

In my opinion, it doesn't make much sense to store the wallet id just to give the user an error when it doesn't match.

@clemahieu
Copy link
Contributor

LGTM

@lukealonso
Copy link
Contributor

LGTM as well

@argakiig argakiig merged commit d6594d5 into nanocurrency:master Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukealonso lukealonso lukealonso left review comments

@cryptocode cryptocode cryptocode left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@PlasmaPower @lukealonso @clemahieu @cryptocode @argakiig