rpc: keep `.cookie` file if it was not generated #28784

romanz · 2023-11-03T15:30:33Z

Otherwise, starting bitcoind twice may cause the .cookie file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).

Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).

DrahtBot · 2023-11-03T15:30:38Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	kristapsk, stickies-v, willcl-ark, achow101
Stale ACK	luke-jr, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28946 (init: don't delete PID file if it was not generated by willcl-ark)
#28167 (init: Add option for rpccookie permissions (replace 26088) by willcl-ark)

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.

romanz · 2023-11-03T15:41:05Z

The bug can be reproduced manually by starting bitcoind twice, resulting in a missing .cookie file:

$ bitcoind -signet &
...
2023-11-03T15:38:09Z Using random cookie authentication.
2023-11-03T15:38:09Z Generated RPC authentication cookie /home/user/.bitcoin/signet/.cookie
...

$ ls ~/.bitcoin/signet/.cookie 
/home/user/.bitcoin/signet/.cookie

$ bitcoind -signet
Error: Cannot obtain a lock on data directory /home/user/.bitcoin/signet. Bitcoin Core is probably already running.

$ ls /home/user/.bitcoin/signet/.cookie 
ls: cannot access '/home/user/.bitcoin/signet/.cookie': No such file or directory

luke-jr

utACK, though it might be better to do something like keep the file open and detect if it's the same file/content before we delete it.

kristapsk

utACK 7cb9367

luke-jr

utACK after squash

Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock). Github-Pull: bitcoin#28784 Rebased-From: 7cb9367

Github-Pull: bitcoin#28784 Rebased-From: ad37ad4

romanz · 2023-11-09T03:50:37Z

Thanks for the review!
Squashed and force-pushed d95dde9.

kristapsk

re-ACK d95dde9

luke-jr · 2023-11-09T22:57:24Z

re-ACK d95dde9

Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock). Github-Pull: bitcoin#28784 Rebased-From: d95dde9

romanz · 2023-11-16T18:44:59Z

Just wanted to ask whether there are any other outstanding issues? :)

romanz · 2023-11-24T19:15:38Z

Ping :)

willcl-ark

ACK d95dde9

Kind of suprised this wasn't noticed earlier, but thanks for the patch!

I also noticed while testing this that the same issue is present with the pid file, but I will open a seperate pull for that in a moment.

maflcko · 2023-11-27T10:35:58Z

lgtm ACK d95dde9

src/rpc/request.cpp

maflcko · 2023-11-30T19:34:05Z

Note: If you revert to 7cb9367 instead (which is the same), the previous reviews will be valid again

romanz · 2023-11-30T19:46:12Z

Note: If you revert to 7cb9367 instead (which is the same), the previous reviews will be valid again

Thanks - reverted to 7cb9367.

kristapsk

re-ACK 7cb9367

stickies-v

ACK 7cb9367

stickies-v · 2023-11-30T23:39:07Z

src/rpc/request.cpp

@@ -131,7 +134,10 @@ bool GetAuthCookie(std::string *cookie_out)
 void DeleteAuthCookie()
 {
    try {
-        fs::remove(GetAuthCookieFile());
+        if (g_generated_cookie) {


nit: can be done without extra nesting

git diff on 7cb9367

diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index b7acd62ee3..ad9f244f12 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -133,11 +133,10 @@ bool GetAuthCookie(std::string *cookie_out) void DeleteAuthCookie() { + // Don't delete the cookie file if it wasn't generated by this process + if (!g_generated_cookie) return; try { - if (g_generated_cookie) { - // Delete the cookie file if it was generated by this process - fs::remove(GetAuthCookieFile()); - } + fs::remove(GetAuthCookieFile()); } catch (const fs::filesystem_error& e) { LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e)); }

willcl-ark · 2023-12-01T11:29:38Z

re-ACK 7cb9367

achow101 · 2023-12-01T17:17:46Z

ACK 7cb9367

romanz · 2023-12-01T18:33:45Z

Thanks :)

8f6ab31 init: don't delete PID file if it was not generated (willcl-ark) Pull request description: In a similar vein to #28784, if a second `bitcoind` is started using the same datadir it will fail to start up, but during shutdown remove the PID file from the first `bitcoind` instance. ACKs for top commit: achow101: ACK 8f6ab31 andrewtoth: ACK 8f6ab31 romanz: ACK 8f6ab31 Tree-SHA512: c9af703cbfa179d33ef9580a51e86c1b0acbd28daa18c8d2e5e5ff796ab4d3e2009a962a47e6046a0e5ece936f8a06ee8af5fdf8ff4ae1e52cbcdbec4b942271

Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock). Github-Pull: bitcoin#28784 Rebased-From: 7cb9367

Github-Pull: bitcoin#28784 Rebased-From: d95dde9

Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock). Github-Pull: bitcoin#28784 Rebased-From: 7cb9367

… generated") 1e95643 rpc: keep .cookie if it was not generated (Roman Zeyde) Pull request description: v26 introduced a regression in that starting a `bitcoind` twice may have the second instance delete the cookie file of the first, making it impossible to communicate with it. Not a big deal but it's annoying, only an issue for 26.0, and the patch is trivial. ACKs for top commit: glozow: lgtm ACK 1e95643 Tree-SHA512: 0e4b18aebaaf284944f1709b238c8c0acce5e8997409e0c278a5a30ac221ac1ff1d3ad31fbf2ac15b03bf7582891e07a7a2cf00f13cb596aa9512566b9320c23

glozow · 2024-03-06T17:07:45Z

this was backported for 26.x in #29503

Github-Pull: bitcoin#28784 Rebased-From: d95dde9

rpc: keep .cookie if it was not generated

7cb9367

Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).

DrahtBot added the RPC/REST/ZMQ label Nov 3, 2023

romanz changed the title ~~rpc: keep .cookie if it was not generated~~ rpc: keep .cookie file if it was not generated Nov 3, 2023

DrahtBot mentioned this pull request Nov 3, 2023

init: Add option for rpccookie permissions (replace 26088) #28167

Merged

luke-jr approved these changes Nov 8, 2023

View reviewed changes

kristapsk approved these changes Nov 8, 2023

View reviewed changes

DrahtBot requested a review from luke-jr November 8, 2023 16:00

luke-jr approved these changes Nov 8, 2023

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 9, 2023

fixup! rpc: keep .cookie if it was not generated

80cb773

Github-Pull: bitcoin#28784 Rebased-From: ad37ad4

romanz force-pushed the fix-cookie-delete branch from ad37ad4 to d95dde9 Compare November 9, 2023 03:49

DrahtBot added CI failed and removed CI failed labels Nov 9, 2023

kristapsk approved these changes Nov 9, 2023

View reviewed changes

DrahtBot requested a review from luke-jr November 9, 2023 05:49

DrahtBot removed the request for review from luke-jr November 9, 2023 22:57

luke-jr approved these changes Nov 9, 2023

View reviewed changes

maflcko modified the milestone: 27.0 Nov 17, 2023

willcl-ark approved these changes Nov 27, 2023

View reviewed changes

willcl-ark mentioned this pull request Nov 27, 2023

init: don't delete PID file if it was not generated #28946

Merged

stickies-v reviewed Nov 27, 2023

View reviewed changes

src/rpc/request.cpp Outdated Show resolved Hide resolved

romanz force-pushed the fix-cookie-delete branch from d95dde9 to 9133b20 Compare November 30, 2023 19:26

romanz force-pushed the fix-cookie-delete branch from 9133b20 to 7cb9367 Compare November 30, 2023 19:45

kristapsk approved these changes Nov 30, 2023

View reviewed changes

DrahtBot requested review from luke-jr, willcl-ark and maflcko November 30, 2023 20:41

stickies-v approved these changes Nov 30, 2023

View reviewed changes

DrahtBot removed the request for review from willcl-ark December 1, 2023 11:29

achow101 merged commit 18bed14 into bitcoin:master Dec 1, 2023
20 checks passed

romanz deleted the fix-cookie-delete branch December 1, 2023 18:33

jp1ac4 mentioned this pull request Jan 19, 2024

[GUI] Use -rpcauth option for Liana-managed bitcoind instead of cookie path wizardsardine/liana#929

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 26, 2024

RPC: Keep .cookie if it was replaced after being generated

33c0e8a

Github-Pull: bitcoin#28784 Rebased-From: d95dde9

achow101 added the Needs backport (26.x) label Feb 22, 2024

achow101 added this to the 26.1 milestone Feb 22, 2024

glozow removed the Needs backport (26.x) label Feb 28, 2024

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

RPC: Keep .cookie if it was replaced after being generated

709e47a

Github-Pull: bitcoin#28784 Rebased-From: d95dde9

This was referenced Jun 11, 2024

bitcoind connection check failed: wizardsardine/liana#1056

Closed

Bug macos Signet setup Recory path multisigner in new wallet wizardsardine/liana#928

Closed

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

RPC: Keep .cookie if it was replaced after being generated

4842324

Github-Pull: bitcoin#28784 Rebased-From: d95dde9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: keep `.cookie` file if it was not generated #28784

rpc: keep `.cookie` file if it was not generated #28784

romanz commented Nov 3, 2023

DrahtBot commented Nov 3, 2023 •

edited

Loading

romanz commented Nov 3, 2023

luke-jr left a comment

kristapsk left a comment

luke-jr left a comment

romanz commented Nov 9, 2023

kristapsk left a comment

luke-jr commented Nov 9, 2023

romanz commented Nov 16, 2023

romanz commented Nov 24, 2023

willcl-ark left a comment

maflcko commented Nov 27, 2023

maflcko commented Nov 30, 2023

romanz commented Nov 30, 2023

kristapsk left a comment

stickies-v left a comment

stickies-v Nov 30, 2023 •

edited

Loading

willcl-ark commented Dec 1, 2023

achow101 commented Dec 1, 2023

romanz commented Dec 1, 2023

glozow commented Mar 6, 2024

rpc: keep .cookie file if it was not generated #28784

rpc: keep .cookie file if it was not generated #28784

Conversation

romanz commented Nov 3, 2023

DrahtBot commented Nov 3, 2023 • edited Loading

Code Coverage

Reviews

Conflicts

romanz commented Nov 3, 2023

luke-jr left a comment

Choose a reason for hiding this comment

kristapsk left a comment

Choose a reason for hiding this comment

luke-jr left a comment

Choose a reason for hiding this comment

romanz commented Nov 9, 2023

kristapsk left a comment

Choose a reason for hiding this comment

luke-jr commented Nov 9, 2023

romanz commented Nov 16, 2023

romanz commented Nov 24, 2023

willcl-ark left a comment

Choose a reason for hiding this comment

maflcko commented Nov 27, 2023

maflcko commented Nov 30, 2023

romanz commented Nov 30, 2023

kristapsk left a comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

willcl-ark commented Dec 1, 2023

achow101 commented Dec 1, 2023

romanz commented Dec 1, 2023

glozow commented Mar 6, 2024

rpc: keep `.cookie` file if it was not generated #28784

rpc: keep `.cookie` file if it was not generated #28784

DrahtBot commented Nov 3, 2023 •

edited

Loading

stickies-v Nov 30, 2023 •

edited

Loading