rpc: Relock wallet only if most recent callback #18814

promag · 2020-04-29T09:48:15Z

This PR fixes an early relocking race condition from #18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time

Issue introduced in #18487.
Fixes #18811.

DrahtBot · 2020-04-29T10:56:02Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: Fix gui shutdown when waitfor* cmds are called from RPC console #18452 (qt: Fix shutdown when waitfor* cmds are called from RPC console by hebasto)
Use shared pointers only in validation interface #18354 (Protect wallet by using shared pointers by bvbfan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/wallet/rpcwallet.cpp

maflcko · 2020-04-29T12:22:24Z

Would be nice to write a test for this

promag · 2020-04-29T12:38:19Z

Thanks for reviewing @ryanofsky. I'll address that case.

@MarcoFalke probably a unit test no?

maflcko · 2020-04-29T12:56:33Z

Yes, I haven't looked at the code changes, but I assume some kind of unit test makes sense.

maflcko · 2020-04-29T13:17:44Z

Also, this needs backport, right?

ryanofsky

Code review ACK b0f1e19. I think all the problems we've seen so far are avoided:

Original deadlock from #18487 where rpcRunLater blocks waiting for old relock callback in timer thread which is deadlocked waiting for cs_wallet
Early relocking race condition from #18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time
Failure to relock race condition in 5a0486c where two relock callbacks could each cancel the other out without either of them relocking

Everything would be simpler if rpcRunLater would just activate the new callback after the old callback if the old callback happened to be running, instead of blocking waiting for the old callback to finish. Then walletpassphrase could go back to using cs_wallet and multiple mutexes wouldn't be required. But I think the approach in this PR should work too and is probably a smaller change on top of existing code.

Only additional suggestion would be to write a PR description that summarizes what this is fixing without having to read the other thread. A unit test could be a good idea, but might be difficult to write, so I don't think it would be crazy to merge this without a new test if it's difficult to come up with one. Especially since the bug this is fixing was caught by a python test.

promag · 2020-04-29T21:58:19Z

A unit test could be a good idea, but might be difficult to write

FWIW I'm still trying this.

promag · 2020-04-29T22:11:22Z

instead of blocking waiting for the old callback to finish.

BTW If we bump minimum libevent then we could use this https://github.com/libevent/libevent/blob/29cc8386a2f7911eaa9336692a2c5544d8b4734f/whatsnew-2.1.txt#L248-L265

maflcko · 2020-04-29T22:30:49Z

Bumping libevent should be discussed separately. See also #18676

ryanofsky · 2020-04-29T22:37:00Z

Also, I haven't looked but I doubt there's a need for a special libevent feature to improve rpcRunLater. We should just need an extra bit of code at the end of the existing libevent callback to register a new callback if there's one pending

promag · 2020-05-03T22:34:06Z

FWIW I'm still trying this.

And I quit. Tried with unit test but it required to start HTTP/RPC server because of libevent and timer stuff. Then tried funcional test but can't figure how to hit the problem.

@ryanofsky I'll borrow your comment above to improve PR description.

ryanofsky · 2020-05-07T00:38:32Z

This still looks good but it might be simpler to squash second and third commits to avoid having a bug in the second commit which is fixed by the third commit.

This would simplify the PR description so you could drop the paragraph "Failure to relock race condition in 59a50e7" about the temporary bug.

promag · 2020-05-07T00:45:21Z

Done.

ryanofsky · 2020-05-07T01:04:38Z

Code review ACK 9f59dde. No changes since last review except squashing commits.

Two other thoughts:

It might be nice to add a comment for RPCRunLater that mentions the risk of deadlocks:

/** 
 * Run func nSeconds from now. 
 * Overrides previous timer <name> (if any).
 *
 * If previous timer has started executing, this will block waiting for the call
 * to return. Therefore, to avoid deadlocks, RPCRunLater can't be called while
 * holding locks that a previous timer might be waiting for.
 *
 * @todo Make implementation nonblocking to avoid risk of deadlocks and simplify
 * usage.
 */ 
void RPCRunLater(const std::string& name, std::function<void()> func, int64_t nSeconds);

This is an idea for a new PR, but maybe there should be an RPC function to return a timer's scheduled time so we could drop CWallet::nRelockTime

maflcko · 2020-05-11T14:45:16Z

ACK 9f59dde

jonatack

ACK 9f59dde

I like @ryanofsky's suggestions in #18814 (comment).

promag · 2020-05-13T10:01:13Z

@ryanofsky honestly I'd suggest to avoid using it. Instead of timeout what I think would be best is to exclusively "bind" the unlock to the current RPC connection. In the GUI we could have a timer. I don't see other uses of the timer in the server.

9f59dde rpc: Relock wallet only if most recent callback (João Barbosa) a2e6db5 rpc: Add mutex to guard deadlineTimers (João Barbosa) Pull request description: This PR fixes an early relocking race condition from bitcoin#18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time Issue introduced in bitcoin#18487. Fixes bitcoin#18811. ACKs for top commit: MarcoFalke: ACK 9f59dde ryanofsky: Code review ACK 9f59dde. No changes since last review except squashing commits. jonatack: ACK 9f59dde Tree-SHA512: 2f7fc03e5ab6037337f2d82dfad432495cc337c77d07c968ee2355105db6292f24543c03456f5402e0e759577a4327758f9372f7ea29de6d56dc3695fda9b379

Github-Pull: bitcoin#18814 Rebased-From: a2e6db5

Github-Pull: bitcoin#18814 Rebased-From: 9f59dde

Github-Pull: bitcoin#18814 Rebased-From: a2e6db5

Github-Pull: bitcoin#18814 Rebased-From: 9f59dde

245c862 test: disable script fuzz tests (fanquake) 9a8fb4c fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer (practicalswift) 6161c94 [net processing] Only send a getheaders for one block in an INV (John Newbery) cf2a6e2 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan) cc7d344 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke) 37a6207 test: Add unregister_validation_interface_race test (MarcoFalke) ff4dc20 gui: Fix manual coin control with multiple wallets loaded (João Barbosa) ed0afe8 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) 251e321 rpc: Relock wallet only if most recent callback (João Barbosa) ca4dac4 rpc: Add mutex to guard deadlineTimers (João Barbosa) a3fe458 [docs] Improve commenting in ProcessGetData() (John Newbery) 011532e [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar) 1e73d72 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar) fb82173 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar) 315ae14 gui: Fix itemWalletAddress leak when not tree mode (João Barbosa) Pull request description: Backports the following PRs to the 0.20 branch: * #18578: gui: Fix leak in CoinControlDialog::updateView * #18808: [net processing] Drop unknown types in getdata * #18814: rpc: Relock wallet only if most recent callback * #18878: test: Add test for conflicted wallet tx notifications * #18894: gui: Fix manual coin control with multiple wallets loaded * #18742: miner: Avoid stack-use-after-return in validationinterface * #18962: net processing: Only send a getheaders for one block in an INV * #18975: test: Remove const to work around compiler error on xenial ACKs for top commit: promag: Tested ACK 245c862 coin control with multiple wallets. laanwj: ACK 245c862 MarcoFalke: ACK 245c862 solved the conflicts myself as a sanity check. Did not re-review 🍷 Tree-SHA512: 285e5a5fad5bbeba6032742c65dc68836e8eccfcceda9e69fec4ddd162a3f61679a96f9bbe3d434267835af67c21ac4c05accf6f63e827c2eb47203c6daafe31

Summary: ``` This PR fixes an early relocking race condition from #18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time ``` Backport of core [[bitcoin/bitcoin#18814 | PR18814]]. Fix spurious wallet_descriptor.py failure like https://build.bitcoinabc.org/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=167449&_focus=3291&guest=1. Test Plan: With Clang: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8803

Github-Pull: #18814 Rebased-From: a2e6db5

Github-Pull: #18814 Rebased-From: 9f59dde

fanquake added the RPC/REST/ZMQ label Apr 29, 2020

rpc: Add mutex to guard deadlineTimers

a2e6db5

promag force-pushed the 2020-04-rpc-wallet-relock branch from 69e21bb to 5a0486c Compare April 29, 2020 10:48

This was referenced Apr 29, 2020

rpc: replace raw pointers with shared_ptrs #18592

Closed

rpc: Fix gui shutdown when waitfor* cmds are called from RPC console #18452

Merged

ryanofsky reviewed Apr 29, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 29, 2020

Use shared pointers only in validation interface #18354

Closed

maflcko added this to the 0.20.0 milestone Apr 29, 2020

maflcko added the Needs backport (0.20) label Apr 29, 2020

promag force-pushed the 2020-04-rpc-wallet-relock branch from 5a0486c to b0f1e19 Compare April 29, 2020 20:34

ryanofsky approved these changes Apr 29, 2020

View reviewed changes

DrahtBot mentioned this pull request May 6, 2020

rpc: prevent circular deps by extracting into separate include #18871

Closed

rpc: Relock wallet only if most recent callback

9f59dde

promag force-pushed the 2020-04-rpc-wallet-relock branch from b0f1e19 to 9f59dde Compare May 7, 2020 00:42

jonatack reviewed May 12, 2020

View reviewed changes

fanquake merged commit a33901c into bitcoin:master May 13, 2020

promag deleted the 2020-04-rpc-wallet-relock branch May 13, 2020 09:55

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 14, 2020

rpc: Add mutex to guard deadlineTimers

0342f4a

Github-Pull: bitcoin#18814 Rebased-From: a2e6db5

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 14, 2020

rpc: Relock wallet only if most recent callback

2e31113

Github-Pull: bitcoin#18814 Rebased-From: 9f59dde

fanquake removed the Needs backport (0.20) label May 14, 2020

fanquake mentioned this pull request May 14, 2020

[0.20] Final backports for rc2 #18973

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020

rpc: Add mutex to guard deadlineTimers

ca4dac4

Github-Pull: bitcoin#18814 Rebased-From: a2e6db5

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020

rpc: Relock wallet only if most recent callback

251e321

Github-Pull: bitcoin#18814 Rebased-From: 9f59dde

backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021

rpc: Add mutex to guard deadlineTimers

6adf27d

Github-Pull: #18814 Rebased-From: a2e6db5

backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021

rpc: Relock wallet only if most recent callback

f75b8d2

Github-Pull: #18814 Rebased-From: 9f59dde

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

rpc: Relock wallet only if most recent callback #18814

rpc: Relock wallet only if most recent callback #18814

Uh oh!

Conversation

promag commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

maflcko commented Apr 29, 2020

Uh oh!

promag commented Apr 29, 2020

Uh oh!

maflcko commented Apr 29, 2020

Uh oh!

maflcko commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Apr 29, 2020

Uh oh!

promag commented Apr 29, 2020

Uh oh!

maflcko commented Apr 29, 2020

Uh oh!

ryanofsky commented Apr 29, 2020

Uh oh!

promag commented May 3, 2020

Uh oh!

ryanofsky commented May 7, 2020

Uh oh!

promag commented May 7, 2020

Uh oh!

ryanofsky commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 11, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented May 13, 2020

Uh oh!

Uh oh!

promag commented Apr 29, 2020 •

edited

Loading

DrahtBot commented Apr 29, 2020 •

edited

Loading

maflcko commented Apr 29, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky commented May 7, 2020 •

edited

Loading