💬 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?
💬 fanquake commented on pull request "guix: combine and document `enable_werror`":
(https://github.com/bitcoin/bitcoin/pull/27326#issuecomment-1483802220)
> as it was when you introduced those "multiple funcs", they will be useful again.
It was needed when we were building 2 different glibcs, in two different ways. Now we're building a single glibc, in one way, hence consolidating. We can speculate about what might happen in future, but I can't see a reason (looking at current releases), to return to multiple builds. The only reason it was done this way initially, was for RISC-V support. I also don't know why we'd turn -Werror back on again, it's
...
(https://github.com/bitcoin/bitcoin/pull/27326#issuecomment-1483802220)
> as it was when you introduced those "multiple funcs", they will be useful again.
It was needed when we were building 2 different glibcs, in two different ways. Now we're building a single glibc, in one way, hence consolidating. We can speculate about what might happen in future, but I can't see a reason (looking at current releases), to return to multiple builds. The only reason it was done this way initially, was for RISC-V support. I also don't know why we'd turn -Werror back on again, it's
...
💬 Ayms commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483821598)
@1440000bytes
> dont get mad else some people from this repo might write things about you. Getting mad is only reserved for a few people here
What do you mean and what is your problem? Write things about me?
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483821598)
@1440000bytes
> dont get mad else some people from this repo might write things about you. Getting mad is only reserved for a few people here
What do you mean and what is your problem? Write things about me?
💬 1440000bytes commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483822456)
@Ayms Edited. Sorry it wasn't for you.
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483822456)
@Ayms Edited. Sorry it wasn't for you.
💬 vincenzopalazzo commented on issue "core stops to run with `Failed to read block` error":
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1483839533)
yeah I will try asap
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1483839533)
yeah I will try asap
💬 hebasto commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483841839)
> I'm seeing an additional unrelated problem with linking with system clang, but I'll PR the solution to that separately as it's not as straightforward as this.
FWIW, with the following diff:
```diff
--- a/depends/hosts/darwin.mk
+++ b/depends/hosts/darwin.mk
@@ -38,7 +38,9 @@ clangxx_prog=$(shell $(SHELL) $(.SHELLFLAGS) "command -v clang++")
llvm_config_prog=$(shell $(SHELL) $(.SHELLFLAGS) "command -v llvm-config")
clang_resource_dir=$(shell clang -print-resource-dir)
+llvm_bin_d
...
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483841839)
> I'm seeing an additional unrelated problem with linking with system clang, but I'll PR the solution to that separately as it's not as straightforward as this.
FWIW, with the following diff:
```diff
--- a/depends/hosts/darwin.mk
+++ b/depends/hosts/darwin.mk
@@ -38,7 +38,9 @@ clangxx_prog=$(shell $(SHELL) $(.SHELLFLAGS) "command -v clang++")
llvm_config_prog=$(shell $(SHELL) $(.SHELLFLAGS) "command -v llvm-config")
clang_resource_dir=$(shell clang -print-resource-dir)
+llvm_bin_d
...
📝 martinus opened a pull request: "refactor: extract CCheckQueue's data handling into a separate container "Bag""
(https://github.com/bitcoin/bitcoin/pull/27331)
`CCheckQueue` has stored its work items in a `queue`, but made no guarantee about the order of elements in that container. This PR extracts that data storage handling into a separate container class `Bag`. This is pure refactoring, the result should have a better separation of concerns, adds tests for the new container, and makes it now easier to separately test and optimize the container.
(https://github.com/bitcoin/bitcoin/pull/27331)
`CCheckQueue` has stored its work items in a `queue`, but made no guarantee about the order of elements in that container. This PR extracts that data storage handling into a separate container class `Bag`. This is pure refactoring, the result should have a better separation of concerns, adds tests for the new container, and makes it now easier to separately test and optimize the container.