💬 nulladoses commented on issue "bitcoin.service issue for a noob like me":
(https://github.com/bitcoin/bitcoin/issues/31021#issuecomment-2390575627)
thank you for your answer. i found also this looking around
[Unit]
Description=Bitcoin Daemon
After=network.target
[Service]
User=ubuntu
PIDFile=/home/ubuntu/.bitcoin/bitcoind.pid
ExecStart=/usr/local/bin/bitcoind
Restart=always
TimeoutSec=120
RestartSec=30
[Install]
WantedBy=multi-user.target
and give me one error when i boot the pc but restart anyway and then is ok
thank you
``
(https://github.com/bitcoin/bitcoin/issues/31021#issuecomment-2390575627)
thank you for your answer. i found also this looking around
[Unit]
Description=Bitcoin Daemon
After=network.target
[Service]
User=ubuntu
PIDFile=/home/ubuntu/.bitcoin/bitcoind.pid
ExecStart=/usr/local/bin/bitcoind
Restart=always
TimeoutSec=120
RestartSec=30
[Install]
WantedBy=multi-user.target
and give me one error when i boot the pc but restart anyway and then is ok
thank you
``
⚠️ nulladoses reopened an issue: "bitcoin.service issue for a noob like me"
(https://github.com/bitcoin/bitcoin/issues/31021)
### Motivation
Hi i don't know if i'm in the right section but i have a problem installing contrib/init/bitcoind.service. i'm a noob so please if someone can help me. I downloaded bitcoin core full node and Tor. Now i installed the service to start bitcoin core on boot of my machine that is Linux Mint on an old macbook pro.
When i make sudo systemctl start bitcoind show like this
### Motivation
Hi i don't know if i'm in the right section but i have a problem installing contrib/init/bitcoind.service. i'm a noob so please if someone can help me. I downloaded bitcoin core full node and Tor. Now i installed the service to start bitcoin core on boot of my machine that is Linux Mint on an old macbook pro.
When i make sudo systemctl start bitcoind show like this
![Schermata del 2024-10-02 19-15-59](https://github.com/user-attachments/assets/b781e7c5-9a29-440e-9492-03cf329e35
...
👍 hodlinator approved a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2344839855)
re-ACK 98c1536852d1de9a978b11046e7414e79ed40b46
`git range-diff master b6368fc`:
- Removed default parameter value in `ParseVerbosity` and added `/*default_verbosity=*/` at callsites.
- Added test for `verbosity=-1`.
- Removed "txid"-comment.
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2344839855)
re-ACK 98c1536852d1de9a978b11046e7414e79ed40b46
`git range-diff master b6368fc`:
- Removed default parameter value in `ParseVerbosity` and added `/*default_verbosity=*/` at callsites.
- Added test for `verbosity=-1`.
- Removed "txid"-comment.
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1785772207)
There are multiple places in which we evict outbound connections. For example, https://github.com/bitcoin/bitcoin/blob/cfb59da4b3bb34afae467691a3e901f2b5a186f3/src/net_processing.cpp#L6291-L6293
Based on earlier review, I renamed the tracepoint to explicitly mention "inbound" - I think the same is fine for the documentation. If we were going to add (an) outbound eviction tracepoint(s), then they'd get their own tracepoint documentation.
See also discussion in https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1785772207)
There are multiple places in which we evict outbound connections. For example, https://github.com/bitcoin/bitcoin/blob/cfb59da4b3bb34afae467691a3e901f2b5a186f3/src/net_processing.cpp#L6291-L6293
Based on earlier review, I renamed the tracepoint to explicitly mention "inbound" - I think the same is fine for the documentation. If we were going to add (an) outbound eviction tracepoint(s), then they'd get their own tracepoint documentation.
See also discussion in https://github.com/bitcoin/bit
...
🤔 ismaelsadeeq reviewed a pull request: "Fix unsigned integer overflows in interpreter"
(https://github.com/bitcoin/bitcoin/pull/24214#pullrequestreview-2344951444)
Code review ACK bbbbaa0d9ac9ae9c9b8109503aa30213eed543b9
> This is safe because signed integer overflow (UB) isn't expected here with 64-bit integers.
If I understand correctly, when two large 64-bit integers are added, they may likely result in signed integer overflow. What you meant here is that the sum of the stack size and the value passed to the "stack macros" is always small enough to prevent signed integer overflow?
From my understanding, this is correct because `MAX_STACK_SI
...
(https://github.com/bitcoin/bitcoin/pull/24214#pullrequestreview-2344951444)
Code review ACK bbbbaa0d9ac9ae9c9b8109503aa30213eed543b9
> This is safe because signed integer overflow (UB) isn't expected here with 64-bit integers.
If I understand correctly, when two large 64-bit integers are added, they may likely result in signed integer overflow. What you meant here is that the sum of the stack size and the value passed to the "stack macros" is always small enough to prevent signed integer overflow?
From my understanding, this is correct because `MAX_STACK_SI
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785842290)
Because if stored by wtxid then the same could happen, but the attacker malleates the transaction to a valid witness data, so it has same txid, different wtxid, and is valid. When we receive that transaction we would not recognize it because we look at the wtxid, so we would keep broadcasting the original one, until the code in `PeerManagerImpl::ReattemptPrivateBroadcast()` gives up on it because it is not acceptable in the mempool.
Do you think we should check both txid and wtxid when we rec
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785842290)
Because if stored by wtxid then the same could happen, but the attacker malleates the transaction to a valid witness data, so it has same txid, different wtxid, and is valid. When we receive that transaction we would not recognize it because we look at the wtxid, so we would keep broadcasting the original one, until the code in `PeerManagerImpl::ReattemptPrivateBroadcast()` gives up on it because it is not acceptable in the mempool.
Do you think we should check both txid and wtxid when we rec
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785884527)
> I'm not sure if the rpc help is the best place to give motivation for why features exist
Right. I dropped some of the text. I will apply further rewording if you suggest some.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785884527)
> I'm not sure if the rpc help is the best place to give motivation for why features exist
Right. I dropped some of the text. I will apply further rewording if you suggest some.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785885728)
Changed, thanks!
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785885728)
Changed, thanks!
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785886009)
Removed.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785886009)
Removed.
🤔 hodlinator reviewed a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2344877863)
Concept ACK 1ba225c6ce689defb7063dd68f62e1e90fcdda5e
Good to see these globals implemented for bench. As already mentioned, I've been working on a partially overlapping change in #30737.
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2344877863)
Concept ACK 1ba225c6ce689defb7063dd68f62e1e90fcdda5e
Good to see these globals implemented for bench. As already mentioned, I've been working on a partially overlapping change in #30737.
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785805254)
nit: Redundant comment.
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785805254)
nit: Redundant comment.
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785776060)
[Braces on new lines for classes, functions, methods.](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c)
Can see you are following style of function above, but function below follows the dev-notes.
Don't feel as strongly about capitalization.
```suggestion
static std::vector<std::string> ParseTestSetupArgs(const ArgsManager& argsman)
{
```
If you want to make `parsePriorityLevel` conform to the dev-notes as well, I support it.
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785776060)
[Braces on new lines for classes, functions, methods.](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c)
Can see you are following style of function above, but function below follows the dev-notes.
Don't feel as strongly about capitalization.
```suggestion
static std::vector<std::string> ParseTestSetupArgs(const ArgsManager& argsman)
{
```
If you want to make `parsePriorityLevel` conform to the dev-notes as well, I support it.
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785821577)
Why not make `SetupUnitTestArgs` available and call it here instead? Delegates the responsibility of documenting the default value to that part of the code.
(Could maybe rename `SetupUnitTestArgs` -> `SetupTestArgs` or `static BasicTestingSetup::RegisterArgs` to make it less unit test-specific and communicate how broadly it is used).
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785821577)
Why not make `SetupUnitTestArgs` available and call it here instead? Delegates the responsibility of documenting the default value to that part of the code.
(Could maybe rename `SetupUnitTestArgs` -> `SetupTestArgs` or `static BasicTestingSetup::RegisterArgs` to make it less unit test-specific and communicate how broadly it is used).
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785827487)
(Please either remove empty line after local global here or add an empty line after `static std::string g_running_benchmark_name`).
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785827487)
(Please either remove empty line after local global here or add an empty line after `static std::string g_running_benchmark_name`).
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785892026)
> it would be better not to do change addrman based on what happens in private connections
I agree! Changed to avoid the `Good()` and `Connected()` calls for private broadcast.
On the second problem - I agree that more tried peers (which happens naturally over time) and more people using private broadcast would help because then the recipient just sees multiple broadcasts and has no way to relate them.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785892026)
> it would be better not to do change addrman based on what happens in private connections
I agree! Changed to avoid the `Good()` and `Connected()` calls for private broadcast.
On the second problem - I agree that more tried peers (which happens naturally over time) and more people using private broadcast would help because then the recipient just sees multiple broadcasts and has no way to relate them.
💬 fanquake commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2390910998)
https://cirrus-ci.com/task/5934269598531584?logs=ci#L2683:
```bash
[14:05:20.035] 131/136 Test #132: wallet_tests ......................... Passed 16.54 sec
[14:05:27.163] 132/136 Test #122: coinselector_tests ................... Passed 35.31 sec
[14:07:02.612] 133/136 Test #6: tests ................................ Passed 262.37 sec
[14:07:04.392] 134/136 Test #9: addrman_tests ........................ Passed 264.14 sec
[14:10:59.874] 135/136 Test #1: util_test_runner
...
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2390910998)
https://cirrus-ci.com/task/5934269598531584?logs=ci#L2683:
```bash
[14:05:20.035] 131/136 Test #132: wallet_tests ......................... Passed 16.54 sec
[14:05:27.163] 132/136 Test #122: coinselector_tests ................... Passed 35.31 sec
[14:07:02.612] 133/136 Test #6: tests ................................ Passed 262.37 sec
[14:07:04.392] 134/136 Test #9: addrman_tests ........................ Passed 264.14 sec
[14:10:59.874] 135/136 Test #1: util_test_runner
...
💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785885817)
This won't define the c++ macro `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`, as this only sets a cmake build flag (cmake probably complains that this flag doesn't exist).
You should be able to use `-DAPPEND_CPPFLAGS="-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"` (I see where the confusion comes from, as `-D` is accepted by compilers to define macros and by cmake to set build flags but they are different things).
You will also need to add this to the correct section in the docs (i.e. the net
...
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785885817)
This won't define the c++ macro `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`, as this only sets a cmake build flag (cmake probably complains that this flag doesn't exist).
You should be able to use `-DAPPEND_CPPFLAGS="-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"` (I see where the confusion comes from, as `-D` is accepted by compilers to define macros and by cmake to set build flags but they are different things).
You will also need to add this to the correct section in the docs (i.e. the net
...
💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785875497)
I think this could be de-duplicated and made a little cleaner (same for the magic byte check):
```c++
bool checksum_ok{memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0};
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
checksum_ok = checksum_ok || (hdr.pchChecksum[0] & 1) != 0;
#endif
if (!checksum_ok) {
...
```
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785875497)
I think this could be de-duplicated and made a little cleaner (same for the magic byte check):
```c++
bool checksum_ok{memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0};
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
checksum_ok = checksum_ok || (hdr.pchChecksum[0] & 1) != 0;
#endif
if (!checksum_ok) {
...
```
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2390929867)
Tested and mostly happy with the first two preparation commits. Note to other reviewers: see discussion in https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610 for why the prefixes are hardcoded instead of derived.
The main commit fd02e7650f6410824a64f1403a98583b7ec6b0dd might be easier to review if you split it up. For example one commit that improves the base58 vs bech32 detection, another commit that that improves base58 error handling and finally one for bech32.
Re last
...
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2390929867)
Tested and mostly happy with the first two preparation commits. Note to other reviewers: see discussion in https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610 for why the prefixes are hardcoded instead of derived.
The main commit fd02e7650f6410824a64f1403a98583b7ec6b0dd might be easier to review if you split it up. For example one commit that improves the base58 vs bech32 detection, another commit that that improves base58 error handling and finally one for bech32.
Re last
...
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785858113)
7478ac6af922c302e292ace7f61e6eaa461da727: this change belongs in the previous commit 07f9733e6022d77d755321914a2a2a1dd7ce0d59
(One way to move it there is by doing `git rebase -i HEAD~3`, setting "e" for the first commit, and then add these lines. Then you do a `git commit -a --amend` to update the first commit. Then `git rebase --continue`, which should automatically adjust the second commit to omit this change.)
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785858113)
7478ac6af922c302e292ace7f61e6eaa461da727: this change belongs in the previous commit 07f9733e6022d77d755321914a2a2a1dd7ce0d59
(One way to move it there is by doing `git rebase -i HEAD~3`, setting "e" for the first commit, and then add these lines. Then you do a `git commit -a --amend` to update the first commit. Then `git rebase --continue`, which should automatically adjust the second commit to omit this change.)
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785860610)
@maflcko I'm puzzled why the `test each commit` job didn't catch this. No test coverage? Shouldn't it use `-Werror`?
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785860610)
@maflcko I'm puzzled why the `test each commit` job didn't catch this. No test coverage? Shouldn't it use `-Werror`?