💬 glozow commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2237009418)
post merge ACK
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2237009418)
post merge ACK
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683160882)
Of course
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683160882)
Of course
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683164631)
Ah, my mistake, this was an intermediary commit - fine either way
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683164631)
Ah, my mistake, this was an intermediary commit - fine either way
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2237035374)
ACK d7f94e0fa9ec3641405e6909ab7da4e48865e840
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2237035374)
ACK d7f94e0fa9ec3641405e6909ab7da4e48865e840
👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2186375491)
re tACK 5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec
Thank you for your commit redo in a more granular way. Just a couple of questions really in the discussion above. To be thorough, rebuilt and retested the commit and all tests included the extended suite pass.
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2186375491)
re tACK 5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec
Thank you for your commit redo in a more granular way. Just a couple of questions really in the discussion above. To be thorough, rebuilt and retested the commit and all tests included the extended suite pass.
🤔 glozow reviewed a pull request: "test: expand LimitOrphan and EraseForPeer coverage"
(https://github.com/bitcoin/bitcoin/pull/30082#pullrequestreview-2186380821)
reACK 172c1ad026cc38c6f52679e74c14579ecc77c48e
(https://github.com/bitcoin/bitcoin/pull/30082#pullrequestreview-2186380821)
reACK 172c1ad026cc38c6f52679e74c14579ecc77c48e
💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683190037)
Measuring exec/s without lazy init:

With:

(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683190037)
Measuring exec/s without lazy init:

With:

💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683143984)
There are more message types that are allowed after the version message and before the verack (some of them are also allowed post verack), essentially all of the ones processed in [this block](https://github.com/bitcoin/bitcoin/blob/20ccb30b7af1c30cece2b0579ba88aa101583f07/src/net_processing.cpp#L3894-L4061) (`wtxidrelay`, `sendaddrv2`, `sendtxrcncl`, `sendheaders`, `sendcmpct`).
I used `ALL_NET_MESSAGE_TYPES`, so that we can avoid touching this test if we add more handshake related message t
...
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683143984)
There are more message types that are allowed after the version message and before the verack (some of them are also allowed post verack), essentially all of the ones processed in [this block](https://github.com/bitcoin/bitcoin/blob/20ccb30b7af1c30cece2b0579ba88aa101583f07/src/net_processing.cpp#L3894-L4061) (`wtxidrelay`, `sendaddrv2`, `sendtxrcncl`, `sendheaders`, `sendcmpct`).
I used `ALL_NET_MESSAGE_TYPES`, so that we can avoid touching this test if we add more handshake related message t
...
💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683123261)
My idea was that this might help a fuzzer to more realistically simulate how time would change between messages. Setting it directly would of course also work but I thought it might result in more mutations that are completely bogus (the time isn't attacker controlled so this seemed less useful). Happy to change it, probably doesn't make a huge difference. What do you think?
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683123261)
My idea was that this might help a fuzzer to more realistically simulate how time would change between messages. Setting it directly would of course also work but I thought it might result in more mutations that are completely bogus (the time isn't attacker controlled so this seemed less useful). Happy to change it, probably doesn't make a huge difference. What do you think?
💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2237077920)
Rebased after #30356 landed. Changed `fees_before` to be a reference to `CAmount` based on the above experience of using it.
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2237077920)
Rebased after #30356 landed. Changed `fees_before` to be a reference to `CAmount` based on the above experience of using it.
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2237085603)
Rebased after #30356 (`#include` order).
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2237085603)
Rebased after #30356 (`#include` order).
💬 Sjors commented on pull request "guix: bump time-machine to e6e70abe65850da0ae69428654375d367088ef5e":
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2237090936)
guix hashes:
```
365b4e5e4f876a949834d6f444edb079851e7e77b4fc77f7d6bafd1958f87a10 guix-build-314407a2fb80/output/aarch64-linux-gnu/SHA256SUMS.part
d8f7f8a9d84e37df272a53e5a69f4325196dfc7d43b1a2c12415e81a140f80ad guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu-debug.tar.gz
aaafb96f75597be1206329b5632e32d1f844e15e69415184e8aada3397789bc6 guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu.tar.gz
d3eec1297168c6104
...
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2237090936)
guix hashes:
```
365b4e5e4f876a949834d6f444edb079851e7e77b4fc77f7d6bafd1958f87a10 guix-build-314407a2fb80/output/aarch64-linux-gnu/SHA256SUMS.part
d8f7f8a9d84e37df272a53e5a69f4325196dfc7d43b1a2c12415e81a140f80ad guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu-debug.tar.gz
aaafb96f75597be1206329b5632e32d1f844e15e69415184e8aada3397789bc6 guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu.tar.gz
d3eec1297168c6104
...
💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2237092200)
Rebased after https://github.com/bitcoin/bitcoin/pull/30356 landed.
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2237092200)
Rebased after https://github.com/bitcoin/bitcoin/pull/30356 landed.
💬 theuni commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683210749)
There are a few places here that seem to be changing behavior from setting flags to adding flags. Are the outcomes guaranteed to be the same?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683210749)
There are a few places here that seem to be changing behavior from setting flags to adding flags. Are the outcomes guaranteed to be the same?
👍 maflcko approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2185698999)
ACK f3632d53a5be7efe61be8181ff481d30f7313bd4 🚲
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK f3632d53a5be7efe61be8181ff
...
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2185698999)
ACK f3632d53a5be7efe61be8181ff481d30f7313bd4 🚲
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK f3632d53a5be7efe61be8181ff
...
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682767124)
q in 0b1960f1b29cfe5209ac68102c8643fc9553f247: Any reason to assert this but not the that the other `m_print_to_*` fields are false? What is the difference of a developer writing `PushBackCallback(...); DisableLogging()` to crash the program vs a developer writing `m_print_to_console=true;DisableLogging()` to achieve the same?
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682767124)
q in 0b1960f1b29cfe5209ac68102c8643fc9553f247: Any reason to assert this but not the that the other `m_print_to_*` fields are false? What is the difference of a developer writing `PushBackCallback(...); DisableLogging()` to crash the program vs a developer writing `m_print_to_console=true;DisableLogging()` to achieve the same?
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683194609)
nit in a67baea866719837865864a5eb56a47a9f132de1: Could switch the two lines and use `std::move`?
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683194609)
nit in a67baea866719837865864a5eb56a47a9f132de1: Could switch the two lines and use `std::move`?
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683186853)
q in a67baea866719837865864a5eb56a47a9f132de1: Why use `str` here? Seems odd to possibly leak terminal control codes into the log?
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683186853)
q in a67baea866719837865864a5eb56a47a9f132de1: Why use `str` here? Seems odd to possibly leak terminal control codes into the log?
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682785077)
style nit in 3854484b988dfc7144e3ed3ca09de563e435cc34:
`1'000'000`
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682785077)
style nit in 3854484b988dfc7144e3ed3ca09de563e435cc34:
`1'000'000`
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683221641)
> I'm surprised looking at the implementation of SetHex to see how unreliable it is in general. Since it can't report errors it winds up just ignoring unexpected input:
Yes, those are known shortcomings. The function will be removed completely in the future, by this will be done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683221641)
> I'm surprised looking at the implementation of SetHex to see how unreliable it is in general. Since it can't report errors it winds up just ignoring unexpected input:
Yes, those are known shortcomings. The function will be removed completely in the future, by this will be done in a follow-up.