👍 TheCharlatan approved a pull request: "depends: qrencode 4.1.1"
(https://github.com/bitcoin/bitcoin/pull/27312)
Code review ACK eb1c3adf38cb71c3e6f298a61871738c4919b4a1
I checked that the additional cflags indeed suppress errors when building with `clang-15`. Also went though the changed configure flags and did a quick sanity check in the gui.
(https://github.com/bitcoin/bitcoin/pull/27312)
Code review ACK eb1c3adf38cb71c3e6f298a61871738c4919b4a1
I checked that the additional cflags indeed suppress errors when building with `clang-15`. Also went though the changed configure flags and did a quick sanity check in the gui.
📝 mxaddict opened a pull request: "Added clangd .cache and compile_commands.json to .gitignore"
(https://github.com/bitcoin/bitcoin/pull/27329)
Added cause these were always getting created whenever I was using clangd in the project
(https://github.com/bitcoin/bitcoin/pull/27329)
Added cause these were always getting created whenever I was using clangd in the project
💬 willcl-ark commented on pull request "Added clangd .cache and compile_commands.json to .gitignore":
(https://github.com/bitcoin/bitcoin/pull/27329#issuecomment-1483411858)
Concept NACK.
See https://github.com/bitcoin/bitcoin/pull/27275 for how this should be done.
(https://github.com/bitcoin/bitcoin/pull/27329#issuecomment-1483411858)
Concept NACK.
See https://github.com/bitcoin/bitcoin/pull/27275 for how this should be done.
💬 mzumsande commented on issue "test_bitcoin: ./chain.h:261: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed.":
(https://github.com/bitcoin/bitcoin/issues/27320#issuecomment-1483430905)
maybe not a cosmic ray after all (#26613)
(https://github.com/bitcoin/bitcoin/issues/27320#issuecomment-1483430905)
maybe not a cosmic ray after all (#26613)
💬 mxaddict commented on pull request "Added clangd .cache and compile_commands.json to .gitignore":
(https://github.com/bitcoin/bitcoin/pull/27329#issuecomment-1483467694)
>
Makes sense
(https://github.com/bitcoin/bitcoin/pull/27329#issuecomment-1483467694)
>
Makes sense
✅ mxaddict closed a pull request: "Added clangd .cache and compile_commands.json to .gitignore"
(https://github.com/bitcoin/bitcoin/pull/27329)
(https://github.com/bitcoin/bitcoin/pull/27329)
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136045597)
nit, it would be more logical for this check to be just after `to_process_count` is incremented below.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136045597)
nit, it would be more logical for this check to be just after `to_process_count` is incremented below.
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136047228)
nit, I think you could eliminate the `to_process_count` variable, and just use the vector size instead.
```suggestion
for (size_t i{0}; i < clustered_txs.size(); ++i) {
```
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136047228)
nit, I think you could eliminate the `to_process_count` variable, and just use the vector size instead.
```suggestion
for (size_t i{0}; i < clustered_txs.size(); ++i) {
```
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136051666)
That's fine, suggestion withdrawn :)
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136051666)
That's fine, suggestion withdrawn :)
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136048292)
nit, I think you can remove this code. (It's nice to eliminate special-case code, not only for simplicity, but to reduce testing.)
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136048292)
nit, I think you can remove this code. (It's nice to eliminate special-case code, not only for simplicity, but to reduce testing.)
💬 theuni commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483641234)
> #27314?
I'm not sure I see the connection?
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483641234)
> #27314?
I'm not sure I see the connection?
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1483714301)
Updated changes:
- Moving the uri parsing validation from the http_request_cb to the HTTPRequest constructor, while doing this realised that it's better to validate the result of `evhttp_request_get_uri()` also at that time.
@vasild, @stickies-v: please let me know what you think and I'll update title & description of the PR and will separate the code in 2 commits (proper fix & non-functional optimisation).
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1483714301)
Updated changes:
- Moving the uri parsing validation from the http_request_cb to the HTTPRequest constructor, while doing this realised that it's better to validate the result of `evhttp_request_get_uri()` also at that time.
@vasild, @stickies-v: please let me know what you think and I'll update title & description of the PR and will separate the code in 2 commits (proper fix & non-functional optimisation).
💬 petertodd commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483723052)
@ajtowns
> NACK. If someone wants to allow a bare OP_RETURN and nothing more they can set `-datacarriersize=1`. There's no reason to ignore someone setting `-nodatacarrier` to indicate they don't want OP_RETURN outputs in their mempool.
Like I said, bare `OP_RETURN` outputs are *not* data carrying outputs. Treating them as such is misleading.
Second, if you are going to take that attitude, why do we even have the `-nodatacarrier` option? Setting `-datacarriersize=0` will also prevent ba
...
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483723052)
@ajtowns
> NACK. If someone wants to allow a bare OP_RETURN and nothing more they can set `-datacarriersize=1`. There's no reason to ignore someone setting `-nodatacarrier` to indicate they don't want OP_RETURN outputs in their mempool.
Like I said, bare `OP_RETURN` outputs are *not* data carrying outputs. Treating them as such is misleading.
Second, if you are going to take that attitude, why do we even have the `-nodatacarrier` option? Setting `-datacarriersize=0` will also prevent ba
...
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1148304529)
@mzumsande, I think you are missing my point, let me try to rephrase it: now on `master`, from `ThreadOpenConnections()` we call `Select()`. With the parent of this PR, we will be calling `Select(network)` which will bring some security (good) + some slowdown (bad).
Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.
What
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1148304529)
@mzumsande, I think you are missing my point, let me try to rephrase it: now on `master`, from `ThreadOpenConnections()` we call `Select()`. With the parent of this PR, we will be calling `Select(network)` which will bring some security (good) + some slowdown (bad).
Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.
What
...
👍 vasild approved a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214)
ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
Maybe other reviewers would be interested in the performance discussion:
https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1130844842
(https://github.com/bitcoin/bitcoin/pull/27214)
ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
Maybe other reviewers would be interested in the performance discussion:
https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1130844842
💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1483728729)
@ajtowns
> > this PR proposes deleting an option that .. (2) does no harm to the node operator.
>
> This isn't true. Running a non-default mempool makes it easier to put different txs in your mempool compared to the majority of the network, which:
>
> * makes it easier to fingerprint your node and its network peers
How specifically does turning *on* full-rbf enable that attack?
> * creates pinning vectors
Can you explain what exactly you mean by this in the context of fu
...
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1483728729)
@ajtowns
> > this PR proposes deleting an option that .. (2) does no harm to the node operator.
>
> This isn't true. Running a non-default mempool makes it easier to put different txs in your mempool compared to the majority of the network, which:
>
> * makes it easier to fingerprint your node and its network peers
How specifically does turning *on* full-rbf enable that attack?
> * creates pinning vectors
Can you explain what exactly you mean by this in the context of fu
...
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1148307887)
@brunoerg, yes, that should still bring some improvement. But only push it into the vector if we have visited all addresses in that bucket and found 0 from the requested network because it may happen that we visit a bucket with an address from the requested network, but only pick it up on some of the subsequent iterations, not from the first one. Change it to `std::unordered_set` so that lookup is fast (`O(1)`).
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1148307887)
@brunoerg, yes, that should still bring some improvement. But only push it into the vector if we have visited all addresses in that bucket and found 0 from the requested network because it may happen that we visit a bucket with an address from the requested network, but only pick it up on some of the subsequent iterations, not from the first one. Change it to `std::unordered_set` so that lookup is fast (`O(1)`).
💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1483735264)
@ariard
> * I think the conciliation of the 0confs use-case with new policy regime like nversion=3 has not been very discussed (e.g I believe 0conf software might have to upgrade themselves to reject future nversion=v3 transactions).
To be clear, 0conf users have to "upgrade" every single time mempool policies/conditions change for any non-trivial amount of hashing power. Pretty much any mempool policy change can be exploited to double-spend unconfirmed transactions. That's why the only ent
...
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1483735264)
@ariard
> * I think the conciliation of the 0confs use-case with new policy regime like nversion=3 has not been very discussed (e.g I believe 0conf software might have to upgrade themselves to reject future nversion=v3 transactions).
To be clear, 0conf users have to "upgrade" every single time mempool policies/conditions change for any non-trivial amount of hashing power. Pretty much any mempool policy change can be exploited to double-spend unconfirmed transactions. That's why the only ent
...
💬 kyranjamie commented on pull request "BIP-322 basic support":
(https://github.com/bitcoin/bitcoin/pull/24058#discussion_r1148338733)
I get this value in my implementation, yet it is not reflected in the BIP
(https://github.com/bitcoin/bitcoin/pull/24058#discussion_r1148338733)
I get this value in my implementation, yet it is not reflected in the BIP
💬 hebasto commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483792947)
> Current build (using forced system clang as a test) results in:
>
> > error: unknown argument: '-internal-externc-isystem/opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/include'
Mind providing steps to reproduce it, including your build environment details?
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483792947)
> Current build (using forced system clang as a test) results in:
>
> > error: unknown argument: '-internal-externc-isystem/opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/include'
Mind providing steps to reproduce it, including your build environment details?