🤔 furszy reviewed a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2039716714)
> I wrote a test for RPC signrawtransactionwithkey that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds MAX_STANDARD_SCRIPTSIG_SIZE and if it ever made it into a block, would exceed MAX_SCRIPT_ELEMENT_SIZE
The second call, the one with 16 pubkeys, doesn't entirely succeed. The test isn't checking the error field in the `signrawtransactionwithkey` response, which returns an `'error': 'Push value size limit exceeded'`.
...
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2039716714)
> I wrote a test for RPC signrawtransactionwithkey that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds MAX_STANDARD_SCRIPTSIG_SIZE and if it ever made it into a block, would exceed MAX_SCRIPT_ELEMENT_SIZE
The second call, the one with 16 pubkeys, doesn't entirely succeed. The test isn't checking the error field in the `signrawtransactionwithkey` response, which returns an `'error': 'Push value size limit exceeded'`.
...
💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094859146)
Re ACK for d53d84834747c37f4060a9ef379e0a6b50155246
Pulled, built, ran all unit/functional tests (all passed).
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094859146)
Re ACK for d53d84834747c37f4060a9ef379e0a6b50155246
Pulled, built, ran all unit/functional tests (all passed).
💬 Ferrydh commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094860396)
这是来自QQ邮箱的假期自动回复邮件。
您好，我最近正在休假中，无法亲自回复您的邮件。我将在假期结束后，尽快给您回复。
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094860396)
这是来自QQ邮箱的假期自动回复邮件。
您好，我最近正在休假中，无法亲自回复您的邮件。我将在假期结束后，尽快给您回复。
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2094886890)
`d98ca056a8...bee22409ea`: rebase and address suggestions
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2094886890)
`d98ca056a8...bee22409ea`: rebase and address suggestions
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369829)
Dropped the commit entirely as suggested.
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369829)
Dropped the commit entirely as suggested.
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369909)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369909)
Done as suggested.
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590370012)
Done.
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590370012)
Done.
💬 achow101 commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2094889653)
> i think it's still missing some part, resolving through Google's DNS (which has more verbose error messages than my ISP) gives:
Should be fixed now
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2094889653)
> i think it's still missing some part, resolving through Google's DNS (which has more verbose error messages than my ISP) gives:
Should be fixed now
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590371367)
Reworded the commit message as suggested, but I didn't get this:
> Right now this returns NONE/true when the empty string "" is passed.
Hmm, no? It returns ALL/true when "" is passed, in `master` and in this PR - there is an `if (str.empty() ...` a few lines above.
> Should return false in that case too
You mean to return false when empty string is passed? That would be some further change that maybe makes sense, but is not included in this PR.
> and never return NONE?
It never
...
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590371367)
Reworded the commit message as suggested, but I didn't get this:
> Right now this returns NONE/true when the empty string "" is passed.
Hmm, no? It returns ALL/true when "" is passed, in `master` and in this PR - there is an `if (str.empty() ...` a few lines above.
> Should return false in that case too
You mean to return false when empty string is passed? That would be some further change that maybe makes sense, but is not included in this PR.
> and never return NONE?
It never
...
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590371992)
Yeah, good observation! I tried to reorder the commits but then if the rpc commit is the first one I would have to remove `"0"` from `LOG_CATEGORIES_BY_STR` which is used outside of `GetLogCategory()` and then it becomes difficult to asses the change. So I left it as it is.
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590371992)
Yeah, good observation! I tried to reorder the commits but then if the rpc commit is the first one I would have to remove `"0"` from `LOG_CATEGORIES_BY_STR` which is used outside of `GetLogCategory()` and then it becomes difficult to asses the change. So I left it as it is.
💬 emsit commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2094892382)
With the tool you're using, it's possible to generate an empty block (with a gap), and after removing the condition in the code, even with null (I haven't studied further issues if psz was empty, the block was successfully generated.)
Or would it be possible to write only the testnet4 release date in Psz?
```
./generate-genesis -timestamp $(date +%s) -pubkey 000000000000000000000000000000000000000000000000000000000000000000 -psz " "
Ctrl Hash: 0x00000000320b39907d50bfd4e60d1b767ffbf403ec
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2094892382)
With the tool you're using, it's possible to generate an empty block (with a gap), and after removing the condition in the code, even with null (I haven't studied further issues if psz was empty, the block was successfully generated.)
Or would it be possible to write only the testnet4 release date in Psz?
```
./generate-genesis -timestamp $(date +%s) -pubkey 000000000000000000000000000000000000000000000000000000000000000000 -psz " "
Ctrl Hash: 0x00000000320b39907d50bfd4e60d1b767ffbf403ec
...
🤔 tdb3 reviewed a pull request: "test: Add a few more corner cases to the base58 test suite"
(https://github.com/bitcoin/bitcoin/pull/30035#pullrequestreview-2039756843)
ACK for 9431bc94a19837a0b053fbbde9f5367543a7264e
Built and ran unit tests (all passed).
Left one nit, but the nit is outside the scope of this PR, so is probably better left to a separate PR.
(https://github.com/bitcoin/bitcoin/pull/30035#pullrequestreview-2039756843)
ACK for 9431bc94a19837a0b053fbbde9f5367543a7264e
Built and ran unit tests (all passed).
Left one nit, but the nit is outside the scope of this PR, so is probably better left to a separate PR.
💬 tdb3 commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1590372865)
nit: Probably outside the scope of this PR (this PR is adding tests, so business logic changes are extraneous), but at first glance, seems like these statements could be simplified, since `InsecureRandRange()` can return 0, the `std::string` constructor can handle 0 count, and string operator+ can handle empty string addition. Maybe I'm missing something?
For example:
```diff
diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp
index 49ef9ff5b5..beb5ef3335 100644
--- a/s
...
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1590372865)
nit: Probably outside the scope of this PR (this PR is adding tests, so business logic changes are extraneous), but at first glance, seems like these statements could be simplified, since `InsecureRandRange()` can return 0, the `std::string` constructor can handle 0 count, and string operator+ can handle empty string addition. Maybe I'm missing something?
For example:
```diff
diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp
index 49ef9ff5b5..beb5ef3335 100644
--- a/s
...
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2094893646)
`bee22409ea...df2422c5f9`: fix CI
this compiles locally but not on the CI:
```cpp
ret.emplace_back(category, WillLogCategory(flag));
```
so instead use:
```cpp
ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});
```
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2094893646)
`bee22409ea...df2422c5f9`: fix CI
this compiles locally but not on the CI:
```cpp
ret.emplace_back(category, WillLogCategory(flag));
```
so instead use:
```cpp
ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});
```
💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1590380169)
Thanks for checking @tdb3, the results would be similar, but since I assumed the spaces are rare in reality, I gave it different odds.
In my impl the probability that we won't have any leading (or trailing) spaces was `50% + 50%*10%`, in your impl it's 10%, so spaces would be in most samples.
I'm fine with both.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1590380169)
Thanks for checking @tdb3, the results would be similar, but since I assumed the spaces are rare in reality, I gave it different odds.
In my impl the probability that we won't have any leading (or trailing) spaces was `50% + 50%*10%`, in your impl it's 10%, so spaces would be in most samples.
I'm fine with both.
💬 tdb3 commented on 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#issuecomment-2094905130)
> > Are there any drawbacks to this?
>
> I didn't notice any.
>
> It's worth mentioning that the total amount of data stored in this database is at least two orders of magnitude higher than even 128 MiB file size.
It would be great if there are few/no drawbacks. Do you mind sharing the methods used so far to test this? It would be great to have some data for comparison.
Other questions that come to mind (thinking out loud before I dig deeper or perform testing):
- Does the change
...
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2094905130)
> > Are there any drawbacks to this?
>
> I didn't notice any.
>
> It's worth mentioning that the total amount of data stored in this database is at least two orders of magnitude higher than even 128 MiB file size.
It would be great if there are few/no drawbacks. Do you mind sharing the methods used so far to test this? It would be great to have some data for comparison.
Other questions that come to mind (thinking out loud before I dig deeper or perform testing):
- Does the change
...
💬 tdb3 commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1590383244)
Thanks, that's right (higher probability of no spaces over rand range alone). I don't have a preference, just an observation.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1590383244)
Thanks, that's right (higher probability of no spaces over rand range alone). I don't have a preference, just an observation.
📝 laanwj opened a pull request: "net: Replace libnatpmp with built-in PCP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043)
Continues #30005. Closes #17012..
This PR replaces NAT-PMP port mapping with its successor PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping, support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should othe
...
(https://github.com/bitcoin/bitcoin/pull/30043)
Continues #30005. Closes #17012..
This PR replaces NAT-PMP port mapping with its successor PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping, support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should othe
...
✅ laanwj closed a pull request: "[PoC, nomerge] PCP IPv4 portmap+IPv6 pinhole test"
(https://github.com/bitcoin/bitcoin/pull/30005)
(https://github.com/bitcoin/bitcoin/pull/30005)
💬 laanwj commented on pull request "[PoC, nomerge] PCP IPv4 portmap+IPv6 pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2094906387)
Continued in #30043.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2094906387)
Continued in #30043.