Update wallet docs and examples to persist staged then take_staged

[notmandatory]

Description

Update wallet tests, docs and examples to persist staged changesets then if that succeeds use take_staged to remove them from the Wallet.

Notes to the reviewers

This was triggered by a discussion with @thunderbiscuit during our last rust dev call as the safer/recommended way to persist wallet changes. Only wallet docs, tests, and examples are changed.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[ValuedMammal]

If we agree that this way is safer, I would perhaps change the API from take_staged to clear_stage and have it return (), that way there's no confusion about which method to get the changeset from.

[storopoli]

This could be catch at compile time/publishing docs by:

- ```rust,no_run
+ ```rust

in the docstring.

[thunderbiscuit]

This is better than what was there before so Concept ACK. But I do think it feels/looks odd. You grab the staged changes using .staged() and once the persistence has succeeded you take_staged()? I'd be confused until I confirmed that it's really the only way to do this. I like @ValuedMammal's suggestion. If this is the intended workflow (pull the staged changes, attempt to persist them, if it succeeds then clear your staging area), then clear_staged() might be a good rename of take_staged() (unless there are other intended uses for it?).

Other question: what if something happens between the time you take the staged changes using .staged() and clear the the staging area using .clear_staged() and the changeset has changed? Is that potentially problematic or can this never happen?

[ValuedMammal]

This is better than what was there before so Concept ACK.

I guess I'm not clear on why this is better though. If persistence fails but you've already taken the stage, is it the end of the world? or can you use it to reload the wallet? This way you don't have to remember to clear the stage and hope there's no race condition.

[evanlinjin]

I'm not sure how this is better unless if the caller has a way to handle persistence failures? However I do not think persisting from .staged is a safe API. What if the caller forgets to clear it afterwards? Another problem is what @thunderbiscuit mentioned about the potential race condition.

If I were to create a wallet, if the persistence fails, I would like to prompt the user about the db error and have a button that says (try again). This is to stop the user from doing further wallet actions until the db error resolves (most of the time it's just a lack of storage space?).

In conclusion, I reckon it's better to incentivize callers to do .take_staged before persisting. I would even consider getting rid of .staged.

[thunderbiscuit]

Good points all around. I'm now not as sure it's better than what's currently there. This issue has made me realize I need to sharpen my understanding of the importance/dangers of persisting the changesets (or not persisting them).

What exactly happens if you mess up persistence of your changeset? Am I correct in thinking that the persistence (similar to what was in <0.30) is more for convenience but it not dangerous to loose at all, as in the worse that can happen with a missing or corrupted db is that you have to chuck it and just do a full_scan again to re-create it from scratch? Are there other issues with loosing the persistence? (other than it's important you know that you've lost it because it has privacy implications if you start giving out addresses you've already given out but don't realize because you didn't redo a full_scan).

Note: if this is explained anywhere I wasn't able to find it. A good place for a discussion on changesets would of course be the ADR, but also https://docs.rs/bdk_wallet/1.0.0-alpha.13/bdk_wallet/wallet/type.ChangeSet.html could use a bit of beefing up maybe?

[notmandatory]

On second though, and based on comments above, I may have been over thinking this problem. The risk I had in mind is that a new address is generated but the keychain index isn't persisted and since we already took it from the wallet we can't retry and it never gets persisted so the address gets reused.

But the better way to handle this scenario is to never show any new receive addresses to the user (or broadcast tx with new change address) until the changeset is successfully persisted. The original docs already capture this best practice.

Any opposition to me closing this PR?

[notmandatory]

@evanlinjin fixed this in #1514.