💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2101030451)
I'm not sure I see the value in using an enum (and forcing it to be passed in) over simply defining a constant? If there's more than 1 possible value, sure. But for now, why?
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2101030451)
I'm not sure I see the value in using an enum (and forcing it to be passed in) over simply defining a constant? If there's more than 1 possible value, sure. But for now, why?
✅ achow101 closed an issue: "Consideration of a Code of Conduct and/or written rules for moderation"
(https://github.com/bitcoin/bitcoin/issues/29507)
(https://github.com/bitcoin/bitcoin/issues/29507)
💬 achow101 commented on issue "Consideration of a Code of Conduct and/or written rules for moderation":
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2101078962)
Since there is a draft guidelines and a meta repo with an open issue for discussion, I'm going to close this one.
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2101078962)
Since there is a draft guidelines and a meta repo with an open issue for discussion, I'm going to close this one.
💬 sr-gi commented on pull request "init: Fixes for file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/27539#issuecomment-2101083639)
I've been reviewing #29415 (which is extending the current brain-hurting approach on master), so I ended up considering fixing this. If @Empact is not working on this anymore I may pick it up
(https://github.com/bitcoin/bitcoin/pull/27539#issuecomment-2101083639)
I've been reviewing #29415 (which is extending the current brain-hurting approach on master), so I ended up considering fixing this. If @Empact is not working on this anymore I may pick it up
💬 kosuodhmwa commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2101088644)
ok thx
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2101088644)
ok thx
💬 edilmedeiros commented on pull request "build, test: Remove unused `TIMEOUT` environment variable":
(https://github.com/bitcoin/bitcoin/pull/30063#issuecomment-2101089608)
ACK 189d0da3f6f561c808fdd9fbd4dfd34ccfa23fe1
Kind of tested with
```bash
❯ ./configure --with-boost=/opt/local/libexec/boost/1.78 --disable-wallet --with-gui=no --disable-gui-tests --disable-zmq --disable-man --without-bdb --enable-lcov
❯ make -j11 cov
```
which I guess would cover the `functional_test.info` rule.
(https://github.com/bitcoin/bitcoin/pull/30063#issuecomment-2101089608)
ACK 189d0da3f6f561c808fdd9fbd4dfd34ccfa23fe1
Kind of tested with
```bash
❯ ./configure --with-boost=/opt/local/libexec/boost/1.78 --disable-wallet --with-gui=no --disable-gui-tests --disable-zmq --disable-man --without-bdb --enable-lcov
❯ make -j11 cov
```
which I guess would cover the `functional_test.info` rule.
💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1594425184)
I wanted to keep same name as in `getpeerinfo` but I agree with you about the confusion, I do believe `mapped_asn` would be better because we show the AS number.
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1594425184)
I wanted to keep same name as in `getpeerinfo` but I agree with you about the confusion, I do believe `mapped_asn` would be better because we show the AS number.
💬 theuni commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-2101158662)
@TheCharlatan Mind adding a commit to fixup the includes [as per this conversation](https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-2041704403)?
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-2101158662)
@TheCharlatan Mind adding a commit to fixup the includes [as per this conversation](https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-2041704403)?
💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1594465354)
Yep! From the description:
> The motivation is to be able to use the ::ApplyTapTweak logic outside of signing, e.g. in silent payments when retrieving the private key. Outside of silent payments, having this method would support any future use cases where the tweaked key is needed outside of signing.
This commit was broken out of the silent payments sending PR, because there we need the tweaked `CKey`. However, we do eventually turn it back into a `keypair` when passing it to libsecp, so m
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1594465354)
Yep! From the description:
> The motivation is to be able to use the ::ApplyTapTweak logic outside of signing, e.g. in silent payments when retrieving the private key. Outside of silent payments, having this method would support any future use cases where the tweaked key is needed outside of signing.
This commit was broken out of the silent payments sending PR, because there we need the tweaked `CKey`. However, we do eventually turn it back into a `keypair` when passing it to libsecp, so m
...
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2101185096)
> I'm not sure I see the value in using an enum (and forcing it to be passed in) over simply defining a constant? If there's more than 1 possible value, sure. But for now, why?
This commit is broken out from #28122 , where we add a new value for silent payment addresses (limit of 1024). Here is the commit that adds a new value for silent payments: https://github.com/bitcoin/bitcoin/commit/622c7a98b9f08177a3cfb601306daabb101af1fd
My motivation for this PR is to prep for the silent payments
...
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2101185096)
> I'm not sure I see the value in using an enum (and forcing it to be passed in) over simply defining a constant? If there's more than 1 possible value, sure. But for now, why?
This commit is broken out from #28122 , where we add a new value for silent payment addresses (limit of 1024). Here is the commit that adds a new value for silent payments: https://github.com/bitcoin/bitcoin/commit/622c7a98b9f08177a3cfb601306daabb101af1fd
My motivation for this PR is to prep for the silent payments
...
👍 TheCharlatan approved a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-2046382441)
ACK 75d27fefc7a04ebdda7be5724a014b6a896e7325
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-2046382441)
ACK 75d27fefc7a04ebdda7be5724a014b6a896e7325
💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594504094)
Is JSON/JSONRPC really necessary for the values themselves if the `enum` is of the type `enum class` so that one always has to prepend the type? - `JSONRPCVersion::JSONRPC_1_BTC` vs `JSONRPCVersion::1_BTC`.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594504094)
Is JSON/JSONRPC really necessary for the values themselves if the `enum` is of the type `enum class` so that one always has to prepend the type? - `JSONRPCVersion::JSONRPC_1_BTC` vs `JSONRPCVersion::1_BTC`.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594514450)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594504094
> Is JSON/JSONRPC really necessary for the values themselves if the `enum` is of the type `enum class` so that one always has to prepend the type? - `JSONRPCVersion::JSONRPC_1_BTC` vs `JSONRPCVersion::1_BTC`.
Good point. I'd probably call the constants `V1_LEGACY` and `V2` if renaming.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594514450)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594504094
> Is JSON/JSONRPC really necessary for the values themselves if the `enum` is of the type `enum class` so that one always has to prepend the type? - `JSONRPCVersion::JSONRPC_1_BTC` vs `JSONRPCVersion::1_BTC`.
Good point. I'd probably call the constants `V1_LEGACY` and `V2` if renaming.
💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1594531344)
The reason I added it was to make the following commit, in which every line of code that can be reached with `fKnown==true` is moved into its own function, a mechanical refactor that is easier to review: Lines dependent on `fKnown` move, lines dependent from `!fKnown` stay, lines independent from `fKnown` go into both.
Without it, there would be the question why this block of code does not make it into `AddToBlockFileInfo`.
So the reason was to move the potential behavior change (which would
...
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1594531344)
The reason I added it was to make the following commit, in which every line of code that can be reached with `fKnown==true` is moved into its own function, a mechanical refactor that is easier to review: Lines dependent on `fKnown` move, lines dependent from `!fKnown` stay, lines independent from `fKnown` go into both.
Without it, there would be the question why this block of code does not make it into `AddToBlockFileInfo`.
So the reason was to move the potential behavior change (which would
...
📝 sr-gi opened a pull request: "init: fixes file descriptor accounting"
(https://github.com/bitcoin/bitcoin/pull/30065)
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR).
Redefine some of the constants, plus document what are we accounting for so this can be extended more easily
Fixe
...
(https://github.com/bitcoin/bitcoin/pull/30065)
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR).
Redefine some of the constants, plus document what are we accounting for so this can be extended more easily
Fixe
...
💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1594555157)
Notice how the latter is redundant once we limit `nFD = std::min(FD_SETSIZE, nFD);`. Moving things around, plus accounting for `nBind` (which is currently not being accounted for in master), we get:
```
nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
nMaxConnections = std::min(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE,);
...
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1594555157)
Notice how the latter is redundant once we limit `nFD = std::min(FD_SETSIZE, nFD);`. Moving things around, plus accounting for `nBind` (which is currently not being accounted for in master), we get:
```
nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
nMaxConnections = std::min(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE,);
...
💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1594557872)
Notice how the latter `nMaxConnections` trim is redundant once we limit `nFD = std::min(FD_SETSIZE, nFD);`. Moving things around, plus accounting for `nBind` (which is currently not being accounted for in master), we get:
```
nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
nMaxConnections = std::min(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM
...
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1594557872)
Notice how the latter `nMaxConnections` trim is redundant once we limit `nFD = std::min(FD_SETSIZE, nFD);`. Moving things around, plus accounting for `nBind` (which is currently not being accounted for in master), we get:
```
nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
nMaxConnections = std::min(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM
...
💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101324949)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101324949)
Concept ACK
💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101333852)
I think it is worth mentioning that the current approach is defining the minimum amount of file descriptions required without accounting for a single connection*, this means that if we are at the bare minimum, we will run but won't be able to connect to any nodes. This is consistent with the current logic in master, but I think it's not the way it should be.
I'm happy to add another commit addressing this, but I've rathered start approaching this sticking to the same assumptions as master.
...
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101333852)
I think it is worth mentioning that the current approach is defining the minimum amount of file descriptions required without accounting for a single connection*, this means that if we are at the bare minimum, we will run but won't be able to connect to any nodes. This is consistent with the current logic in master, but I think it's not the way it should be.
I'm happy to add another commit addressing this, but I've rathered start approaching this sticking to the same assumptions as master.
...
💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101340385)
@vasild you may be interested in this. I decided to fix it when seeing you extending the current logic in #29415
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101340385)
@vasild you may be interested in this. I decided to fix it when seeing you extending the current logic in #29415