Page MenuHomePhabricator

[Node Daemon ] Updating the outdated bitcoind.service.
ClosedPublic

Authored by alitayin on Dec 3 2023, 01:57.

Details

Reviewers
Fabien
Mengerian
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC0eb2a28b9ecf: [Node Daemon ] Updating the outdated bitcoind.service.
Summary

Provide a template for the eCash Node Daemon Setup Guide

Test Plan

The tests have been conducted on three different VPSs, each running Ubuntu, and each provided by a different service provider.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25836
Build 51252: Build Diff
Build 51251: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Dec 3 2023, 01:57
alitayin requested review of this revision.Dec 3 2023, 01:57

You should add a test plan.

As far as I know, you used this file to set up the systemd service on a server, so you can write that in the test plan.

Also, I notice that we already have bitcoind.service in this directory. Wouldn't it make more sense to replace or modify that file, rather than ending up with two different but similar versions in here?

Mengerian requested changes to this revision.Dec 3 2023, 04:38
This revision now requires changes to proceed.Dec 3 2023, 04:38
This comment was removed by alitayin.

Following Antony's suggestion, abandon the creation of a new ecashd.service file and instead replace the outdated bitcoind.service.

alitayin retitled this revision from [Node Daemon ] Add template ecashd.service to [Node Daemon ] Updating the outdated bitcoind.service..Dec 3 2023, 12:27
alitayin edited the summary of this revision. (Show Details)
Fabien requested changes to this revision.Dec 3 2023, 15:10
Fabien added inline comments.
contrib/init/bitcoind.service
10–11 ↗(On Diff #43367)

You should not disable the rate limiting completely. If the node refuses to start there is likely a reason and keep retrying infinitely will not help

This revision now requires changes to proceed.Dec 3 2023, 15:10

There's something wrong with this Diff, it's not showing the actual change against master.

You probably need to squash some commits

alitayin edited the summary of this revision. (Show Details)
  1. Modify StartLimitInterval
  2. Modify type
  3. rebase my commit

Update init.md's instructions about systemd

Fabien requested changes to this revision.Dec 5 2023, 07:27
Fabien added inline comments.
contrib/init/bitcoind.service
7 ↗(On Diff #43393)

You should stick to Type=forking here and add the -daemonwait option to bitcoind.

doc/init.md
79 ↗(On Diff #43393)

The systemd service file expects the bitcoind executable to be located in ~/bitcoin-abc/bin/bitcoind, so make sure to create the directory and move or copy the file before running the following instructions.

80 ↗(On Diff #43393)

The ~ is a shortcut for the current user home directory

81 ↗(On Diff #43393)
82 ↗(On Diff #43393)
This revision now requires changes to proceed.Dec 5 2023, 07:27

Modify according to fabien's comments

Fabien requested changes to this revision.Dec 6 2023, 08:49
Fabien added inline comments.
doc/init.md
80

there is a weird char here ?

82

you missed this one

This revision now requires changes to proceed.Dec 6 2023, 08:49

I couldn't find any issues between 'in' and '~'. By the way, I tested -deamonwait on three nodes, and it worked perfectly.

Minor nit, but looks like you're making very wide lines in init.md

I'm not sure what the standard is, but usually something like 80 or 100 characters per line is good. I also wonder why the linter doesn't catch this.

Fabien requested changes to this revision.Dec 7 2023, 13:56

I'm not sure what the standard is, but usually something like 80 or 100 characters per line is good. I also wonder why the linter doesn't catch this.

This not enforced on markdown files.

One extra duplicated line to remove and it's good to go.

doc/init.md
82–84 ↗(On Diff #43446)
This revision now requires changes to proceed.Dec 7 2023, 13:56

Removed extra duplicated text

Tried this out, seems to work.

doc/init.md
86 ↗(On Diff #43500)

I'm not sure what "to enable user login" means in this context.

This revision is now accepted and ready to land.Dec 8 2023, 16:32
Mengerian requested changes to this revision.Dec 8 2023, 17:57

Clarify documentation for systemctl --user enable bitcoind

doc/init.md
86 ↗(On Diff #43500)
This revision now requires changes to proceed.Dec 8 2023, 17:57
Mengerian added inline comments.
doc/init.md
83 ↗(On Diff #43530)

Random blank line here mid-sentence.
Should be removed

This revision is now accepted and ready to land.Dec 10 2023, 03:02
Unknown Object (User) requested changes to this revision.Dec 10 2023, 07:21
Unknown Object (User) added a subscriber: Unknown Object (User).
This comment was removed by Fabien.
This revision now requires changes to proceed.Dec 10 2023, 07:21
Mengerian removed a reviewer: Unknown Object (User).Dec 10 2023, 15:12
This revision is now accepted and ready to land.Dec 10 2023, 15:12