Open
Conversation
Contributor
|
Thanks for the PR. I want to review, but I'm curious, is there any reason to add knots to polar? Are there users who specifically want to use it in Polar? Consider adding a justification to the PR description. It's important to ensure that managing the implementation versions over time is truly worthwhile. |
Author
Owner
|
@kwsantiago Thanks for the contribution. I don't have any objections to adding Bitcoin Knots since the RPC is identical to Bitcoin Core, so there should be very little code changes required for it to work. But I do have some concerns about the ongoing maintenance burden this will add for maintainers (primarily myself) to ensure we add support for future versions of Knots in a timely manner. I'm already behind on this with the 6 implementations we currently support. Would you be willing to commit to helping in this regard for some time? I've tried to review this but I'm facing a few hurdles getting this working:
After facing these issues so far, I'd like to hand this PR back to you to make improvements. Please confirm that you have this feature fully functional on your end, then I can take another pass on it. Thanks again for what you have done so far. |
kwsantiago
force-pushed
the
knots-support
branch
from
febb97d to
3246d68
Compare
Author
Owner
|
@kwsantiago I'm not sure how you are successfully running this on your machine. I'm still facing issues:
Please resolve these errors, provide proper testing instructions and be sure that they work without any issues before requesting another round of review. |
kwsantiago
force-pushed
the
knots-support
branch
3 times, most recently
from
bea93e2 to
279137f
Compare
Author
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1303 +/- ##
===========================================
- Coverage 100.00% 99.58% -0.42%
===========================================
Files 156 156
Lines 5734 5764 +30
Branches 1104 1161 +57
===========================================
+ Hits 5734 5740 +6
- Misses 0 24 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kwsantiago
force-pushed
the
knots-support
branch
from
279137f to
15a7214
Compare
kwsantiago
force-pushed
the
knots-support
branch
from
15a7214 to
318623d
Compare
Author
|
@jamaljsr any update on this? I left testing instructions on the last comment. Please let me know if anything else is needed. |
kwsantiago
force-pushed
the
knots-support
branch
from
318623d to
8b66208
Compare
Greptile SummaryThis PR adds Bitcoin Knots (v26–v29) as a fully supported Bitcoin backend implementation in Polar alongside Bitcoin Core. It introduces a new Docker image ( Key changes:
Critical issue found:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| src/utils/constants.ts | Adds BasePorts['bitcoind-knots'] and dockerConfigs['bitcoind-knots'] — but BasePorts['bitcoind-knots'].rest is set to 18443 (same as bitcoind), which conflicts with test expectations of 18543 and would cause port collisions between two different-implementation networks running simultaneously. |
| src/utils/network.ts | Adds createBitcoindKnotsNetworkNode and updates createNetwork to handle bitcoindKnotsNodes. Implementation closely mirrors createBitcoindNetworkNode. Minor inconsistency: p2p/zmqBlock/zmqTx always use global BasePorts constants regardless of the passed basePort argument (same as bitcoind's pattern). |
| src/store/models/network.ts | Adds bitcoindKnotsNodes to AddNetworkArgs, forwards it in addNetwork, and adds a new case 'bitcoind-knots' in addNode. Uses defensive ` |
| src/components/network/NewNetwork.tsx | Adds bitcoindKnotsNodes input field, adjusts column widths from span={6} to span={4} to fit 5 columns, and replaces the hardcoded min={1} on bitcoindNodes with a JS-level validation requiring at least one Bitcoin backend when Lightning nodes are present. |
| docker/bitcoind-knots/Dockerfile | New Dockerfile that downloads Bitcoin Knots binaries from bitcoinknots.org using build args for version and release date. No checksum verification on the downloaded tarball. Also fetches bash completion scripts from external GitHub URLs at build time. |
| src/shared/types.ts | Adds 'bitcoind-knots' to BitcoinNode['implementation'] union type and introduces a new BitcoindKnotsNode interface. Clean, correct type extension. |
| src/lib/bitcoin/bitcoinFactory.ts | Registers 'bitcoind-knots' in the factory using the existing bitcoindService — correct, since Bitcoin Knots is API-compatible with Bitcoin Core. |
| src/lib/docker/composeFile.ts | Updates addBitcoind to dynamically select image name and default command based on implementation rather than hardcoding 'bitcoind'. This correctly supports both bitcoind and bitcoind-knots. |
| src/store/models/app.ts | Adds 'bitcoind-knots' to newNodeCounts (default 0) and basePorts initial state using BasePorts['bitcoind-knots'].rest. Due to the port mismatch in constants.ts, this will emit 18443 rather than the 18543 expected by the tests. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NewNetwork Form] -->|Submit| B{Validation}
B -->|tapdNodes > lndNodes| E1[Error: tapdCountError]
B -->|hasLightning && !hasBitcoinBackend| E2[Error: bitcoinBackendError]
B -->|Valid| C[addNetwork action]
C --> D[createNetwork]
D --> D1[Create bitcoind nodes\ncreateBitcoindNetworkNode]
D --> D2[Create bitcoind-knots nodes\ncreateBitcoindKnotsNetworkNode]
D1 & D2 --> D3[Peer nodes together\nin bitcoin array]
C --> F[Docker Compose]
F --> G{node.implementation}
G -->|bitcoind| H[polarlightning/bitcoind image]
G -->|bitcoind-knots| I[polarlightning/bitcoind-knots image]
H & I --> J[bitcoindService RPC\nsame API for both]
K[addNode action] -->|type: bitcoind-knots| L[createBitcoindKnotsNetworkNode\nwith settings.basePorts]
L --> D3
Last reviewed commit: 8b66208
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Abdulkbk
suggested changes
Mar 16, 2026
Contributor
Abdulkbk
left a comment
Abdulkbk
left a comment
There was a problem hiding this comment.
Hey @kwsantiago, I looked into this feature, and I have a couple of initial feedback:
- There are a couple of linter issues with some files, for example,
src/components/network/NetworkSetting.tsxandsrc/components/network/NewNetwork.tsx. - Opening a terminal does not work currently.
- I think the logo for knots should be different from that of bitcoind.
- In some places, you use
bitcoind-knots, while in others, you usebitcoindKnots, be consistent. - After creating a network, adding a knot using the drag-and-drop feature does not work.
- Running
yarn cilocally fails.
Generally, I think there's a bit more work needed to bring this to the finish line.
kwsantiago
force-pushed
the
knots-support
branch
from
8732648 to
548fd1c
Compare
Author
|
@Abdulkbk thank you for the feedback, it is greatly appreciated! Addressed all feedback: fixed linter issues, terminal support, drag-and-drop, added distinct Knots logo, updated to v29.3, squashed commits, and yarn ci passes clean. Note: Docker images need to be built and pushed to polarlightning/bitcoind-knots before end-to-end testing. |
Contributor
Oh no, this isn't what I was suggesting. Now I see only one commit, which makes it nearly impossible to review. You should break the commits, like you did earlier. My suggestion was to squash just the last commit into the first. |
kwsantiago
force-pushed
the
knots-support
branch
from
548fd1c to
71968da
Compare
kwsantiago
force-pushed
the
knots-support
branch
from
71968da to
dfa78db
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1302
Description
This PR adds full support for Bitcoin Knots v29 as a Bitcoin backend implementation in Polar, alongside the existing Bitcoin Core support.
What changed:
bitcoindKnotsNodesparameterTechnical details:
'bitcoind-knots'added toBitcoinNodeinterfacecreateBitcoindKnotsNetworkNode()function for network node creationSteps to Test
yarn devScreenshots