net: Prevent node from binding to the same CService#33231
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33231. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Would need to update the doxygen ("A vector...")
|
Concept ACK |
l0rinc
left a comment
There was a problem hiding this comment.
Concept ACK, but approach NACK 132a59698e2df2e25fdbae3a6fba308b131419d2
I only did a code review pass on it, please let me know if my concerns are off, happy to ack if I was mistaken.
My biggest concern currently is silently dropping values by partial duplication, e.g. -whitebind=[::1]:8335=relay -whitebind=[::1]:8335=relay,addr.
There was a problem hiding this comment.
the v prefix doesn't make a lot of sense anymore, if they're not vectors
There was a problem hiding this comment.
- I'd appreciate a test that checks what happens in this case, as @jonatack also mentioned
- a release‑notes line under P2P noting that repeated -bind and -whitebind arguments are now deduplicated
There was a problem hiding this comment.
isn't it confusing that in case of service duplicates the it's not obvious which m_flags is ignored?
What do we expect to happen in case of something like -whitebind=127.0.0.1:8335=relay,noban -whitebind=127.0.0.1:8335=relay vs -whitebind=127.0.0.1:8335=relay -whitebind=127.0.0.1:8335=relay,noban?
Or even worse, -whitebind=127.0.0.1:8335=all -whitebind=127.0.0.1:8335=relay,mempool?
There was a problem hiding this comment.
We could modernize this to C++20
std::weak_ordering operator<=>(NetWhitebindPermissions const& other) constnit: formatting is off
|
Concept ACK One point to consider is a behavioral change in how the -bind=192.167.1.10:8333=onion
-bind=127.0.0.1:8333=onionCurrently One option would be to keep using sets for deduplication, while still preserving the historical “first specified wins” rule when selecting |
|
Since these lists are tiny, Instead of a set for deduplication, we might as well use a simple O^2 algorithm, iterate from the back and remove whatever appeared in the front (it will likely be even faster than a set). If you decide to do that, I'd appreciate a randomized unit test checking it against a set deduplication. |
afcafcf to
93d373a
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
cfdfee3 to
1001b7a
Compare
|
Thank you all for the reviews. While addressing the reviews, I realized that using Therefore, even after deduplicating each vector, it is still necessary to check for conflicts between vectors. I used the following approach: I added the tests as requested. For more explanation about the first commit, see #33250. |
l0rinc
left a comment
There was a problem hiding this comment.
Concept ACK
I personally would not try to correct the behavior (especially silently), given that it's rare, I would simply give the user a warning or error and let them fix the arguments.
I also think the tests are a bit complicated and more stateful now, I have added a few suggestions, maybe we can simplify based on them.
|
|
||
| // Check if any service from vBinds exists in vWhiteBinds | ||
| for (const auto& bind : vBinds) { | ||
| if (whitebind_services.count(bind)) { |
There was a problem hiding this comment.
nit:
| if (whitebind_services.count(bind)) { | |
| if (whitebind_services.contains(bind)) { |
| std::vector<NetWhitebindPermissions>& vWhiteBinds, | ||
| std::vector<CService>& vBinds, | ||
| std::vector<CService>& onion_binds) |
There was a problem hiding this comment.
output parameters are quite hard to reason about - given that the call-site assigns connOptions directly, can we either split this into 2-3 helper methods that return the new values (the vectors are tiny, they're likely as fast as passing it around), or if you insist on this big one, can we pass a single CConnman::Options& connOptions instead?
Also, some branches return, others don't erase, others replace one of the inputs - seems like has more than a single, easily defined responsibility (the name even has an And in the name), I personally would prefer doing the same in more focused methods.
| // Step 2: Simple deduplication for vBinds | ||
| { | ||
| std::set<CService> seen; | ||
| auto it = std::remove_if(vBinds.begin(), vBinds.end(), |
There was a problem hiding this comment.
nit:
| auto it = std::remove_if(vBinds.begin(), vBinds.end(), | |
| auto it = std::ranges::remove_if(onion_binds, |
| continue; | ||
| } else { | ||
| // Same service, different permissions - conflict! | ||
| return wb.m_service; |
There was a problem hiding this comment.
Since we don't expect this to happen often, I guess it's enough to stop at the first error instead of listing all 👍
| } | ||
|
|
||
| // Check if any service from onion_binds exists in vWhiteBinds or vBinds | ||
| std::set<CService> bind_services(vBinds.begin(), vBinds.end()); |
There was a problem hiding this comment.
as mentioned before, I think we can safely check vBinds directly - it's simpler and likely even faster than this
| std::set<CService> whitebind_services; | ||
| for (const auto& wb : vWhiteBinds) { | ||
| whitebind_services.insert(wb.m_service); | ||
| } |
There was a problem hiding this comment.
do we really need to recreate this or can we reuse seen_whitebinds or even better, just iterate vWhiteBinds values directly?
| ], | ||
| ) | ||
| port += 2 | ||
| port += 2 # Increment port to avoid conflicts |
There was a problem hiding this comment.
nit: in first case increment means +1, here it's +1 - but more generally, the code is already, it's enough if we just document the intent:
| port += 2 # Increment port to avoid conflicts | |
| port += 2 # avoid conflicts since we've used two ports before |
or even better, why are we even reusing the same variable, why not create a port iterator and instead of the list a lambda accepting two ports something like (untested, since this test is skipped on a Mac):
def run_test(self):
lp = addr_to_hex("127.0.0.1")
test_cases = [
lambda p, _: ([[f"-bind=127.0.0.1:{p}=onion"], # onion-only
[(lp, p)], "no normal -bind with -bind=...=onion"]),
lambda p, q: ([[f"-bind=127.0.0.1:{p}", f"-bind=127.0.0.1:{q}=onion"], # bind + onion
[(lp, p), (lp, q)], "both -bind and -bind=...=onion"]),
lambda p, _: ([[f"-bind=127.0.0.1:{p}"], # bind only
[(lp, p)], "no -bind=...=onion"]),
lambda p, q: ([[f"-bind=127.0.0.1:{p}", f"-bind=127.0.0.1:{p}", f"-bind=127.0.0.1:{q}"], # duplicated bind
[(lp, p), (lp, p), (lp, q)], "duplicated -bind=... and -bind=..."]),
lambda p, q: ([[f"-bind=127.0.0.1:{p}=onion", f"-bind=127.0.0.1:{p}=onion", f"-bind=127.0.0.1:{q}=onion"], # duplicated onion bind
[(lp, p), (lp, p), (lp, q)], "duplicated -bind=...=onion and -bind=...=onion"]),
]
ports = itertools.count(p2p_port(self.num_nodes))
for i, make in enumerate(test_cases):
args, expected_services, description = make(next(ports), next(ports))
self.log.info(f"Test case {i + 1}: {description}")
self.log.info(f"Restarting node 0 with args: {args}")It's also a bit inconsistent that at the end we just hardcode -bind=127.0.0.1:11012, maybe we could extract that port as well
| [&seen](const CService& service) { | ||
| return !seen.insert(service).second; // Remove if already seen | ||
| }); | ||
| onion_binds.erase(it, onion_binds.end()); |
There was a problem hiding this comment.
I find it a bit inconsistent that we're not erring here - we don't know why they provided the same thing twice, seems like a mistake, not sure we should attempt to correct it silently. Would also simplify the code if we unified the way we handle these
|
|
||
| for (const auto& wb : vWhiteBinds) { | ||
| auto it = seen_whitebinds.find(wb.m_service); | ||
| if (it == seen_whitebinds.end()) { |
There was a problem hiding this comment.
nit: would make sense to invert this if wee keep this format, it makes a bit more sense to use it in the condition where we're checking it (would also enable embedding it in the condition).
Not sure this applies if you remove the reundant deduplication code and use iteration instead of sets. I think that would simplify all of this considerably.
|
|
||
| self.nodes[0].assert_start_raises_init_error( | ||
| ["-bind=127.0.0.1:11012", "-bind=127.0.0.1:11012=onion"], | ||
| "Different binding configurations assigned to the address 127.0.0.1:11012", |
There was a problem hiding this comment.
nit: if it's a regex, we need to escape some chars
|
@l0rinc thank you for your detailed reviews.
I adjusted the approach to meet this requirement. Now when duplicates are detected, the node terminates with a clear, specific error message: "Duplicate binding configuration for address < addr >. Please check your -bind, -whitebind, and onion binding settings." I don't have a strong opinion on whether the node should try to correct the behavior or not. For me, the main issue is the misleading message. This version is much more simplified compared to the previous ones |
| connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", DEFAULT_I2P_ACCEPT_INCOMING); | ||
|
|
||
| if (auto conflict = CheckBindingConflicts(connOptions)) { | ||
| return InitError(Untranslated(strprintf("Duplicate binding configuration for address %s. " |
There was a problem hiding this comment.
Nice and clear! One small UX suggestion: since we already know which container the duplicate came from, it could be helpful to mention the categories in the error (e.g. “-bind vs -whitebind”). That said, even without categories, I think this should at least be translatable for consistency with nearby init errors (e.g. i2psam). Something like:
return InitError(strprintf(
_("Duplicate binding configuration for address %s. "
"Please check your -bind, -whitebind, and onion binding settings."),
conflict->ToStringAddrPort()));There was a problem hiding this comment.
since we already know which container the duplicate came from
That is known inside CheckBindingConflicts() but the info is not signaled to the caller of the function. Also, even inside the function, only one of the containers is known, the other is not. E.g. if the 3rd for-loop finds a duplicate, then it is unknown whether the entry was added in the first or the second for-loop.
I think this should at least be translatable for consistency with nearby init errors
Right, +1 for translating it.
There was a problem hiding this comment.
the info is not signaled to the caller of the function.
Yes, earlier versions of this PR did that, but I simplified it.
I think this should at least be translatable for consistency with nearby init errors
Done in 1c40b32597712059d5d809925da0e9adccac0fb3
There was a problem hiding this comment.
I like this simpler version more, left a few more comments
Now that we're not deduplicating anymore, the title could be adjusted:
net: reject duplicate bind addresses across -bind/-whitebind/onion
The PR desciption also hints that we still do:
This PR proposes that repeated -bind options have no effect.
| self.stop_node(0) | ||
|
|
||
| # Test all combinations of duplicate bindings (each should fail) | ||
| error_msg = "Error: Duplicate binding configuration for address 127.0.0.1:11012. " \ |
There was a problem hiding this comment.
nit: This seems to be used a single time, we might as well inline it
There was a problem hiding this comment.
Done in 1c40b32597712059d5d809925da0e9adccac0fb3. Thanks.
| error_msg, | ||
| match=ErrorMatch.FULL_TEXT) |
There was a problem hiding this comment.
What's the advantage of duplicating the exact error here, wouldn't this suffice?
| error_msg, | |
| match=ErrorMatch.FULL_TEXT) | |
| "Error: Duplicate binding configuration", | |
| match=ErrorMatch.PARTIAL_REGEX) |
There was a problem hiding this comment.
Done in 1c40b32597712059d5d809925da0e9adccac0fb3.
| addr_to_hex, | ||
| get_bind_addrs, | ||
| ) | ||
| from test_framework.test_node import ErrorMatch |
There was a problem hiding this comment.
The doc at the top is slightly off now:
"""
Test starting bitcoind with -bind and/or -bind=...=onion, confirm that
it binds to the expected ports, and verify that duplicate or conflicting
-bind/-whitebind configurations are rejected with a descriptive error.
"""
There was a problem hiding this comment.
Done in 1c40b32597712059d5d809925da0e9adccac0fb3.
| for i, opt1 in enumerate(bind_options): | ||
| for j, opt2 in enumerate(bind_options): | ||
| if i <= j: # Avoid testing reverse duplicates (A,B) and (B,A) |
There was a problem hiding this comment.
we could make this manual iteration more pythony with something like:
from itertools import combinations_with_replacement
addr = f"127.0.0.1:11012"
for opt1, opt2 in combinations_with_replacement([f"-bind={addr}", f"-bind={addr}=onion", f"-whitebind=noban@{addr}"], 2):
print(f"opt1 = {opt1}, opt2 = {opt2}") # replace with assert, I just used this to validate if the iteration is the sameThere was a problem hiding this comment.
Done in 1c40b32597712059d5d809925da0e9adccac0fb3.
| /** | ||
| * @brief Checks for duplicate bindings across all binding configuration. | ||
| * | ||
| * @param connOptions Connection options containing the binding vectors to check | ||
| * @return std::optional<CService> containing the first duplicate found, or std::nullopt if no duplicates | ||
| */ |
There was a problem hiding this comment.
I personally consider these comments noisy - all of this is already evident from the code. If it's not, I'd rather change the code. Seeing the unupdated comment below, we shouldn't just add extra work for our future selves.
There was a problem hiding this comment.
It wasn't clear to me what the return value is before reading the comment, so this is useful. Sometimes comments are more on the side of redundant/noisy/useless, but the line between that and useful comments is blurred and personally I prefer to err on the side of redundancy.
If there are no comments whatsoever, this sets a standard. Then if the function is extended with not-so-obvious parameters or return value changed, the developer doing it will likely not add a comment for the new parameter.
Yes, comments need to be maintained to be in sync with the code. But note that the argument "Lets omit some comments because we will have to maintain them (more future work)" is missing a point - a code is read many more times than it is modified. So it is better have code that is "easier to read and 'harder' to modify" than "harder to read and easier to modify".
There was a problem hiding this comment.
I’ll leave the comment in for now.
I agree that clear, organized code is the best form of documentation, but I think helpful comments can still save valuable developer time.
|
|
||
| connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", DEFAULT_I2P_ACCEPT_INCOMING); | ||
|
|
||
| if (auto conflict = CheckBindingConflicts(connOptions)) { |
There was a problem hiding this comment.
Is it expected that I'm seeing this twice?
2025-08-26T18:23:45Z [error] Duplicate binding configuration for address 127.0.0.1:11012. Please check your -bind, -whitebind, and onion binding settings.
2025-08-26T18:23:45Z tor: Thread interrupt
2025-08-26T18:23:45Z torcontrol thread exit
2025-08-26T18:23:45Z Shutdown in progress...
Error: Duplicate binding configuration for address 127.0.0.1:11012. Please check your -bind, -whitebind, and onion binding settings.
And if we're not actually stopping the execution, shouldn't it rather be a warning?
There was a problem hiding this comment.
I guess the first err msg is from logging while the second is from init.cpp. From the comment #33231 (comment), I can see that we would want to stop the execution.
There was a problem hiding this comment.
The execution will be stopped either way (in the master branch or in this PR).
This PR simply prevents the node from attempting to connect twice to the same address and port, and instead terminates it with a clear message — rather than the confusing one currently shown in the master branch.
I guess the first err msg is from logging while the second is from init.cpp
Yes, that makes sense. I'll check again.
There was a problem hiding this comment.
it just continues IBD on my side after the warning, it doesn't seem to stop after the error
There was a problem hiding this comment.
If you are running in the foreground, InitError() prints the message to the console. In debug.log, only a single entry is recorded.
This behavior applies to all InitError() calls.
bitcoin/src/node/interface_ui.cpp
Line 64 in 6ca6f3b
it just continues IBD on my side after the warning, it doesn't seem to stop after the error
Could you please check again? InitError() returns false, which should stop execution.
Line 237 in 6ca6f3b
Also, the feature_bind_extra test would fail if the node didn’t actually stop.
There was a problem hiding this comment.
it seems to stop for empty blockchain, but it will just continue if something already exists
# build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -i2psam=invalid_addr
...
025-08-27T23:49:17Z Discover: 2a01:4f9:2a:a61::2
2025-08-27T23:49:17Z [error] Invalid -i2psam address or hostname: 'invalid_addr'
Error: Invalid -i2psam address or hostname: 'invalid_addr'
2025-08-27T23:49:17Z tor: Thread interrupt
2025-08-27T23:49:17Z Shutdown in progress...
2025-08-27T23:49:17Z torcontrol thread exit
2025-08-27T23:49:17Z Loaded 0 blocks from external file in 165ms
2025-08-27T23:49:17Z Reindexing block file blk00001.dat...
...
vs
# build/bin/bitcoind -i2psam=invalid_addr
2025-08-27T23:48:51Z Discover: 2a01:4f9:2a:a61::2
2025-08-27T23:48:51Z [error] Invalid -i2psam address or hostname: 'invalid_addr'
Error: Invalid -i2psam address or hostname: 'invalid_addr'
2025-08-27T23:48:51Z tor: Thread interrupt
2025-08-27T23:48:51Z Shutdown in progress...
2025-08-27T23:48:51Z torcontrol thread exit
2025-08-27T23:48:57Z mapport thread exit
2025-08-27T23:48:57Z scheduler thread exit
2025-08-27T23:48:57Z Writing 0 mempool transactions to file...
2025-08-27T23:48:57Z Writing 0 unbroadcast transactions to file.
2025-08-27T23:48:57Z Dumped mempool: 0.000s to copy, 0.051s to dump, 27 bytes dumped to file
2025-08-27T23:48:57Z Flushed fee estimates to fee_estimates.dat.
2025-08-27T23:48:57Z Shutdown done There was a problem hiding this comment.
Was not yet able to reproduce the continuation. But just to clarify, @l0rinc : are we talking about a late-fail with “afterglow” or does the node truly keep running?
There was a problem hiding this comment.
I started it and accidentally left it in the background and it fetched 100k blocks - if it's an "afterglow", it's a resilient one. And I did reproduce it on 3 separate servers, seems like a real bug - though likely not introduced in this PR, will open an issue for it or investigate it myself later this week.
There was a problem hiding this comment.
Curious to see what you find. I’ll try to follow if you open an issue/PR on it.
naiyoma
left a comment
There was a problem hiding this comment.
Concept ACK
Tested with these 3 options:
./build/bin/bitcoind -bind=0.0.0.0=onion -bind=0.0.0.0=onion./build/bin/bitcoind -bind=0.0.0.0 -bind=0.0.0.0./build/bin/bitcoind -whitebind=relay@127.0.0.1:8333 -whitebind=mempool@127.0.0.1:8333
bitcoind shuts down after the error
[error] Duplicate binding configuration for address 0.0.0.0:8334. Please check your -bind, -whitebind, and onion binding settings. Error: Duplicate binding configuration for address 0.0.0.0:8334. Please check your -bind, -whitebind, and onion binding settings. 2025-08-28T13:58:57Z torcontrol thread start 2025-08-28T13:58:57Z tor: Thread interrupt 2025-08-28T13:58:57Z Shutdown in progress...For duplicate onion binds, there's an existing warning that appears before the error:
2025-08-28T13:58:57Z [warning] More than one onion bind address is provided.
IMO this warning is sufficient, but if you keep the error approach, consider removing the warning.
Thanks for the review @naiyoma . Regarding removing the warning, I think the |
| } | ||
|
|
||
| /** | ||
| * @brief Checks for duplicate bindings across all binding configuration. |
There was a problem hiding this comment.
configuration -> configurations [“across all binding configuration” should be pluralized to “configurations” for grammatical correctness]
| /** | ||
| * @brief Checks for duplicate bindings across all binding configuration. | ||
| * | ||
| * @param connOptions Connection options containing the binding vectors to check |
There was a problem hiding this comment.
| * @param connOptions Connection options containing the binding vectors to check | |
| * @param[in] connOptions Connection options containing the binding vectors to check |
| /** | ||
| * @brief Checks for duplicate bindings across all binding configuration. | ||
| * | ||
| * @param connOptions Connection options containing the binding vectors to check | ||
| * @return std::optional<CService> containing the first duplicate found, or std::nullopt if no duplicates | ||
| */ |
There was a problem hiding this comment.
It wasn't clear to me what the return value is before reading the comment, so this is useful. Sometimes comments are more on the side of redundant/noisy/useless, but the line between that and useful comments is blurred and personally I prefer to err on the side of redundancy.
If there are no comments whatsoever, this sets a standard. Then if the function is extended with not-so-obvious parameters or return value changed, the developer doing it will likely not add a comment for the new parameter.
Yes, comments need to be maintained to be in sync with the code. But note that the argument "Lets omit some comments because we will have to maintain them (more future work)" is missing a point - a code is read many more times than it is modified. So it is better have code that is "easier to read and 'harder' to modify" than "harder to read and easier to modify".
| * @param connOptions Connection options containing the binding vectors to check | ||
| * @return std::optional<CService> containing the first duplicate found, or std::nullopt if no duplicates | ||
| */ | ||
| std::optional<CService> CheckBindingConflicts(const CConnman::Options& connOptions) |
There was a problem hiding this comment.
Since the new function is used only in this file:
| std::optional<CService> CheckBindingConflicts(const CConnman::Options& connOptions) | |
| static std::optional<CService> CheckBindingConflicts(const CConnman::Options& connOptions) |
|
|
||
| if (auto conflict = CheckBindingConflicts(connOptions)) { | ||
| return InitError(Untranslated(strprintf("Duplicate binding configuration for address %s. " | ||
| "Please check your -bind, -whitebind, and onion binding settings.", |
There was a problem hiding this comment.
Here and also in the commit message, this lists 3 categories: -bind, -whitebind, onion. There is an -onion config option which is not related to binding. This could be confusing. The intention here is to mean "onion" -> "a subset of -bind if suffixed with =onion". That is, just mentioning -bind also covers -bind=...=onion. So maybe change this to:
Please check your -bind and -whitebind settings or to
Please check your -bind, -bind=...=onion and -whitebind settings.
| connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", DEFAULT_I2P_ACCEPT_INCOMING); | ||
|
|
||
| if (auto conflict = CheckBindingConflicts(connOptions)) { | ||
| return InitError(Untranslated(strprintf("Duplicate binding configuration for address %s. " |
There was a problem hiding this comment.
since we already know which container the duplicate came from
That is known inside CheckBindingConflicts() but the info is not signaled to the caller of the function. Also, even inside the function, only one of the containers is known, the other is not. E.g. if the 3rd for-loop finds a duplicate, then it is unknown whether the entry was added in the first or the second for-loop.
I think this should at least be translatable for consistency with nearby init errors
Right, +1 for translating it.
e534628 to
1c40b32
Compare
There was a problem hiding this comment.
@yuvicc, this still needs fixing
Ahh, I thought that was non-blocking.
yuvicc
left a comment
There was a problem hiding this comment.
ACK 1c40b32597712059d5d809925da0e9adccac0fb3
bitcoind stops with error message for duplicate bind options.
Error: Duplicate binding configuration for address 0.0.0.0:8333. Please check your -bind, -bind=...=onion and -whitebind settings.
2025-09-05T05:15:05Z torcontrol thread start
2025-09-05T05:15:05Z Loading 0 mempool transactions from file...
2025-09-05T05:15:05Z Imported mempool transactions from file: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2025-09-05T05:15:05Z initload thread exit
| * @param connOptions Connection options containing the binding vectors to check | ||
| * @return std::optional<CService> containing the first duplicate found, or std::nullopt if no duplicates | ||
| */ | ||
| std::optional<CService> CheckBindingConflicts(const CConnman::Options& connOptions) |
There was a problem hiding this comment.
| std::optional<CService> CheckBindingConflicts(const CConnman::Options& connOptions) | |
| std::optional<CService> CheckBindingConflicts(const CConnman::Options& conn_options) |
nit: as per the guidelines, function arguments should be snake_case.
Currently, if the user inadvertently starts the node with duplicate bind options, such as `-bind=0.0.0.0 -bind=0.0.0.0`, it will cause a fatal error with the misleading message "Bitcoin Core is probably already running". This commit adds early validation to detect duplicate bindings across all binding configurations (-bind, -whitebind, and onion bindings) before attempting to bind. When duplicates are detected, the node terminates with a clear, specific error message: "Duplicate binding configuration for address <addr>. Please check your -bind, -bind=...=onion and -whitebind settings." The validation catches duplicates both within the same option type (e.g., `-bind=X -bind=X`) and across different types (e.g., `-bind=X -whitebind=Y@X`), helping users identify and fix configuration mistakes.
1c40b32 to
4d4789d
Compare
|
ACK 4d4789d |
|
re-ACK 4d4789d |
naiyoma
left a comment
There was a problem hiding this comment.
Tested ACK 4d4789d
Manually tested with different duplicate bindings.
./build/bin/bitcoind -bind=0.0.0.0=onion -bind=0.0.0.0=onion
./build/bin/bitcoind -bind=0.0.0.0 -bind=0.0.0.0
./build/bin/bitcoind -whitebind=relay@127.0.0.1:8333 -whitebind=mempool@127.0.0.1:8333
On master, the error message is misleading
2025-09-09T10:59:38Z [net:error] Unable to bind to 0.0.0.0:8334 on this computer. Bitcoin Core is probably already running.
2025-09-09T10:59:38Z [error] Unable to bind to 0.0.0.0:8334 on this computer. Bitcoin Core is probably already running.
Error: Unable to bind to 0.0.0.0:8334 on this computer. Bitcoin Core is probably already running.
After this PR change, the error message is much clearer and points to the issue.
2025-09-09T11:02:19Z [error] Duplicate binding configuration for address 0.0.0.0:8334. Please check your -bind, -bind=...=onion and -whitebind settings.
Error: Duplicate binding configuration for address 0.0.0.0:8334. Please check your -bind, -bind=...=onion and -whitebind settings.
|
utACK 4d4789d |
|
ACK 4d4789d |
Currently, if the node inadvertently starts with repeated
-bindoptions (e.g../build/bin/bitcoind -listen -bind=0.0.0.0 -bind=0.0.0.0), the user will receive a misleading message followed by the node shutdown:And the user might spend some time looking for a
bitcoindprocess or what application is using port 8333, when what happens is that Bitcoin Core successfully connected to port 8333 and then tries again, generating this fatal error.This PR proposes that repeated
-bindoptions have no effect.