💬 usuarrio-id81214293 commented on issue "Intermittent failure in feature_fee_estimation.py in sanity_check_rbf_estimates: est_feerate = node.estimatesmartfee(2)["feerate"] (KeyError: 'feerate')":
(https://github.com/bitcoin/bitcoin/issues/30640#issuecomment-2390373539)
cmake -B build -DWITH_ZMQ=ON -DREDUCE_EXPORTS=ON -DCMAKE_C_FLAGS="-g0 -O2 -funsigned-char" -DCMAKE_CXX_FLAGS="-g0 -O2 -funsigned-char -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"
(https://github.com/bitcoin/bitcoin/issues/30640#issuecomment-2390373539)
cmake -B build -DWITH_ZMQ=ON -DREDUCE_EXPORTS=ON -DCMAKE_C_FLAGS="-g0 -O2 -funsigned-char" -DCMAKE_CXX_FLAGS="-g0 -O2 -funsigned-char -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"
💬 luisschwab commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2390376610)
Concept ACK.
Great feature, not being able to do this is a PITA.
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2390376610)
Concept ACK.
Great feature, not being able to do this is a PITA.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785630298)
This code in `CConnman::ThreadOpenConnections()` is not checking whether private broadcast is enabled because it is just a servant, its logic is: "if there is a request to open private broadcast connections, then I will open them". The higher level code is making the decision (currently in RPC only, but eventually in the wallet as well) - "if private broadcast is enabled, then open such connections because I am now broadcasting a transaction (and don't put it in the mempool)". This is why there
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785630298)
This code in `CConnman::ThreadOpenConnections()` is not checking whether private broadcast is enabled because it is just a servant, its logic is: "if there is a request to open private broadcast connections, then I will open them". The higher level code is making the decision (currently in RPC only, but eventually in the wallet as well) - "if private broadcast is enabled, then open such connections because I am now broadcasting a transaction (and don't put it in the mempool)". This is why there
...
✅ nulladoses closed an issue: "bitcoin.service issue for a noob like me"
(https://github.com/bitcoin/bitcoin/issues/31021)
(https://github.com/bitcoin/bitcoin/issues/31021)
💬 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
...