🤔 mzumsande reviewed a pull request: "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate"
(https://github.com/bitcoin/bitcoin/pull/30039#pullrequestreview-2046205765)
I've played around with this branch a bit, upgrading and downgrading between it and master with existing datadirs on signet and didn't run into any issues.
Also just noting that this will affect all leveldb databases, also the indexes and the `block/index` db.
(https://github.com/bitcoin/bitcoin/pull/30039#pullrequestreview-2046205765)
I've played around with this branch a bit, upgrading and downgrading between it and master with existing datadirs on signet and didn't run into any issues.
Also just noting that this will affect all leveldb databases, also the indexes and the `block/index` db.
🤔 glozow reviewed a pull request: "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`"
(https://github.com/bitcoin/bitcoin/pull/29954#pullrequestreview-2046213151)
This seems useful, and makes sense as a field of `getmempoolinfo` like the others
(https://github.com/bitcoin/bitcoin/pull/29954#pullrequestreview-2046213151)
This seems useful, and makes sense as a field of `getmempoolinfo` like the others
💬 glozow commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1594348765)
I slightly prefer it being called datacarriersize to match the config option, but don't feel strongly
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1594348765)
I slightly prefer it being called datacarriersize to match the config option, but don't feel strongly
💬 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
...