📝 hebasto opened a pull request: "Refactor `subprocess.hpp` to follow our conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)
This PR:
1. Replaces a custom `__USING_WINDOWS__` macro with the `WIN32`.
2. Renames the header `*.hpp` --> `*.h` and adjust the header guard name.
A draft for now as it is based on https://github.com/bitcoin/bitcoin/pull/29865 to avoid conflicts.
(https://github.com/bitcoin/bitcoin/pull/29910)
This PR:
1. Replaces a custom `__USING_WINDOWS__` macro with the `WIN32`.
2. Renames the header `*.hpp` --> `*.h` and adjust the header guard name.
A draft for now as it is based on https://github.com/bitcoin/bitcoin/pull/29865 to avoid conflicts.
💬 fanquake commented on pull request "Refactor `subprocess.hpp` to follow our conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066228898)
Why not put the windows change with the windows change: https://github.com/bitcoin/bitcoin/pull/29868 ?
Looks like the other change could just be PR'd on it's not, and doesn't need to be based on anything?
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066228898)
Why not put the windows change with the windows change: https://github.com/bitcoin/bitcoin/pull/29868 ?
Looks like the other change could just be PR'd on it's not, and doesn't need to be based on anything?
👋 hebasto's pull request is ready for review: "refactor: Rename `subprocess.hpp` to follow our header name conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)
(https://github.com/bitcoin/bitcoin/pull/29910)
💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066248047)
> Why not put the windows change with the windows change: #29868 ? Looks like the other change could just be PR'd on it's own, and doesn't need to be based on anything? Avoiding conflicts doesn't seem necessary given this branch already conflicts as-is?
Thanks! Reworked per your suggestions.
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066248047)
> Why not put the windows change with the windows change: #29868 ? Looks like the other change could just be PR'd on it's own, and doesn't need to be based on anything? Avoiding conflicts doesn't seem necessary given this branch already conflicts as-is?
Thanks! Reworked per your suggestions.
💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066251518)
Drafted again until resolving a linter warning.
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066251518)
Drafted again until resolving a linter warning.
📝 hebasto converted_to_draft a pull request: "refactor: Rename `subprocess.hpp` to follow our header name conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)
This PR renames the header `*.hpp` --> `*.h` and adjusts the header guard name.
(https://github.com/bitcoin/bitcoin/pull/29910)
This PR renames the header `*.hpp` --> `*.h` and adjusts the header guard name.
💬 BrandonOdiwuor commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1572143420)
fixed
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1572143420)
fixed
💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066305987)
> Drafted again until resolving a linter warning. (The header is processed by linters now).
Linter warnings have been resolved. But the PR remains as a draft because the https://github.com/bitcoin/bitcoin/pull/29865 will make the diff smaller.
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066305987)
> Drafted again until resolving a linter warning. (The header is processed by linters now).
Linter warnings have been resolved. But the PR remains as a draft because the https://github.com/bitcoin/bitcoin/pull/29865 will make the diff smaller.
⚠️ kcalvinalvin opened an issue: "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does"
(https://github.com/bitcoin/bitcoin/issues/29911)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
It's stated in `chainparams.cpp` that "seed.bitcoinstats.com" supports filtering. However, this isn't true.
https://github.com/bitcoin/bitcoin/blob/c05c214f2e9cfd6070a3c7680bfa09358fd9d97a/src/kernel/chainparams.cpp#L137
### Expected behaviour
Since "seed.bitcoinstats.com" supports filtering for `NODE_NETWORK`, when asked for `NODE_NETWORK`, it should return archival nodes. It doesn't
...
(https://github.com/bitcoin/bitcoin/issues/29911)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
It's stated in `chainparams.cpp` that "seed.bitcoinstats.com" supports filtering. However, this isn't true.
https://github.com/bitcoin/bitcoin/blob/c05c214f2e9cfd6070a3c7680bfa09358fd9d97a/src/kernel/chainparams.cpp#L137
### Expected behaviour
Since "seed.bitcoinstats.com" supports filtering for `NODE_NETWORK`, when asked for `NODE_NETWORK`, it should return archival nodes. It doesn't
...
🤔 glozow reviewed a pull request: "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)"
(https://github.com/bitcoin/bitcoin/pull/29827#pullrequestreview-2011134310)
code review ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493
(https://github.com/bitcoin/bitcoin/pull/29827#pullrequestreview-2011134310)
code review ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493
💬 vasild commented on pull request "addrman: improve performance of `GetAddr` when specifying network":
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2066352274)
The development of Bitcoin Core is decentralized because every developer decides for themselves what is important and what not.
Asserting one's opinion on others is more in line with centralized corporate management.
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2066352274)
The development of Bitcoin Core is decentralized because every developer decides for themselves what is important and what not.
Asserting one's opinion on others is more in line with centralized corporate management.
💬 maflcko commented on pull request "addrman: improve performance of `GetAddr` when specifying network":
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2066381384)
> opinion
Performance improvements should be measured in numbers by machines, which does not seem like an opinion. According to https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2023247803 , the code can only be reached by RPC and P2P. For those places no end-to-end performance difference has been provided. I'd say if there is a measurable (reproducible) performance improvement for a users' use case, or another use case that hasn't been mentioned yet, the change can be considered bas
...
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2066381384)
> opinion
Performance improvements should be measured in numbers by machines, which does not seem like an opinion. According to https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2023247803 , the code can only be reached by RPC and P2P. For those places no end-to-end performance difference has been provided. I'd say if there is a measurable (reproducible) performance improvement for a users' use case, or another use case that hasn't been mentioned yet, the change can be considered bas
...
💬 furszy commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1572234194)
tiny typo
```suggestion
// Only genesis, which must be part of the best header chain, can have a nullptr parent.
```
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1572234194)
tiny typo
```suggestion
// Only genesis, which must be part of the best header chain, can have a nullptr parent.
```
💬 stratospher commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2066393593)
oh interesting! there's some discussion in https://github.com/bitcoin/bitcoin/pull/29809 about how to update the comments for DNS seed filtering. also cc @naiyoma
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2066393593)
oh interesting! there's some discussion in https://github.com/bitcoin/bitcoin/pull/29809 about how to update the comments for DNS seed filtering. also cc @naiyoma
💬 kcalvinalvin commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2066402231)
I guess I should add that I don't think the nodes returned by "seed.bitcoinstats.com" are good.
My assumptions is that it's just caching all the nodes it's previously seen and not checking if they're still providing all the services it previously has.
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2066402231)
I guess I should add that I don't think the nodes returned by "seed.bitcoinstats.com" are good.
My assumptions is that it's just caching all the nodes it's previously seen and not checking if they're still providing all the services it previously has.
⚠️ maflcko opened an issue: "RFC: Formal description of the RPC API"
(https://github.com/bitcoin/bitcoin/issues/29912)
Currently (all?) clients manually implement the RPC API, which is problematic, because:
* It leads to accidental implementation bugs, such as unit mistakes (s/vB vs BTC/kvB)
* It is hard to maintain, when the API changes
* It requires effort to implement in a new programming language
So it could make sense to have a formal specification.
Please leave a comment if there are additional aspects to consider here, or if you have ideas for a solution.
One solution for a formal descriptio
...
(https://github.com/bitcoin/bitcoin/issues/29912)
Currently (all?) clients manually implement the RPC API, which is problematic, because:
* It leads to accidental implementation bugs, such as unit mistakes (s/vB vs BTC/kvB)
* It is hard to maintain, when the API changes
* It requires effort to implement in a new programming language
So it could make sense to have a formal specification.
Please leave a comment if there are additional aspects to consider here, or if you have ideas for a solution.
One solution for a formal descriptio
...
💬 nflatrea commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066435250)
The example you showed doesn't even support C/C++ code-base.
If you need clear documentation / specification on the JSON-RPC API for bitcoincore you have plethora of well documented guides.
https://developer.bitcoin.org/reference/rpc/
https://wbnns.github.io/bitcoin-dev-docs/reference/rpc/index.html
https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md
https://en.bitcoin.it/wiki/API_reference_%28JSON-RPC%29
Considering your need for a state-of-the-art framework / w
...
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066435250)
The example you showed doesn't even support C/C++ code-base.
If you need clear documentation / specification on the JSON-RPC API for bitcoincore you have plethora of well documented guides.
https://developer.bitcoin.org/reference/rpc/
https://wbnns.github.io/bitcoin-dev-docs/reference/rpc/index.html
https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md
https://en.bitcoin.it/wiki/API_reference_%28JSON-RPC%29
Considering your need for a state-of-the-art framework / w
...
📝 furszy opened a pull request: "bugfix: update chainman best_header after block reconsideration"
(https://github.com/bitcoin/bitcoin/pull/29913)
Fixes #26245. I'm doing it because it came to my mind after reviewing #28339.
Inside the `invalidateblock` process, we update the best_header by manually calling `InvalidChainFound` after disconnecting blocks.
We need to do the same for `reconsiderblock` and update the chain `best_header` field after finishing the process.
Note: the only difference between this two commands is that `reconsiderblock` does not have its own separate function, it's a plain RPC command that resets the block
...
(https://github.com/bitcoin/bitcoin/pull/29913)
Fixes #26245. I'm doing it because it came to my mind after reviewing #28339.
Inside the `invalidateblock` process, we update the best_header by manually calling `InvalidChainFound` after disconnecting blocks.
We need to do the same for `reconsiderblock` and update the chain `best_header` field after finishing the process.
Note: the only difference between this two commands is that `reconsiderblock` does not have its own separate function, it's a plain RPC command that resets the block
...
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1572288041)
Added this test and tweaked a bit to check that orphan stays + can still be resolved.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1572288041)
Added this test and tweaked a bit to check that orphan stays + can still be resolved.
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1572288118)
done
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1572288118)
done