💬 fanquake commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1467547077)
These lines have never been super consistent, but are meant to be a rough overview of the (interesting) flags being used for compilation. I think you're correct in that printing secp flags (again) here would be overkill. For any other flags, I don't think they are very meaningful for most builders. I think it's also unlikely we'll significantly change anything here too much further, given that this is going to be completely redone with CMake.
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1467547077)
These lines have never been super consistent, but are meant to be a rough overview of the (interesting) flags being used for compilation. I think you're correct in that printing secp flags (again) here would be overkill. For any other flags, I don't think they are very meaningful for most builders. I think it's also unlikely we'll significantly change anything here too much further, given that this is going to be completely redone with CMake.
✅ fanquake closed an issue: "libsecp256k1 not instrumented when building with sanitizers"
(https://github.com/bitcoin/bitcoin/issues/27990)
(https://github.com/bitcoin/bitcoin/issues/27990)
🚀 fanquake merged a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875)
(https://github.com/bitcoin/bitcoin/pull/28875)
✅ dergoegge closed a pull request: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043)
(https://github.com/bitcoin/bitcoin/pull/28043)
💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1911941270)
It's been more than 6 months since I opened this without substantial review, so closing. Let me know if anyone is actually interested in reviewing and I'll reopen.
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1911941270)
It's been more than 6 months since I opened this without substantial review, so closing. Let me know if anyone is actually interested in reviewing and I'll reopen.
💬 torkelrogstad commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#discussion_r1467562973)
Ah, thanks. I'm very new to C++, so I didn't realize I could even use things that weren't imported. Didn't realize I should also include a header statement, since it compiled just fine! Added the include.
(https://github.com/bitcoin/bitcoin/pull/29175#discussion_r1467562973)
Ah, thanks. I'm very new to C++, so I didn't realize I could even use things that weren't imported. Didn't realize I should also include a header statement, since it compiled just fine! Added the include.
💬 RicYashiroLee commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911955460)
Why was my last comment to Peter Todd's comment, DELETED? Where can I find the reasoning for such deletion?
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911955460)
Why was my last comment to Peter Todd's comment, DELETED? Where can I find the reasoning for such deletion?
💬 glozow commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1467572446)
Is this doing 100 iterations per setting, when it was 1000 before?
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1467572446)
Is this doing 100 iterations per setting, when it was 1000 before?
💬 glozow commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1467560621)
IIUC pseudocode:
Use values of `x` to codify all possibilities of 9 bits, with 1000 iters for each setting. So x=0..(1000*2^9)-1
for `x_pos`, `v_pos` in enumerate([0, 1, 50, 51, 52, 53, 61, 62, 63]):
`v` starts as 64 random bits
reset `v`'s `v_pos`th bit to what `x` 's `x_pos` bit is
turn `v` into double `f` and `TestDouble(f)`
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1467560621)
IIUC pseudocode:
Use values of `x` to codify all possibilities of 9 bits, with 1000 iters for each setting. So x=0..(1000*2^9)-1
for `x_pos`, `v_pos` in enumerate([0, 1, 50, 51, 52, 53, 61, 62, 63]):
`v` starts as 64 random bits
reset `v`'s `v_pos`th bit to what `x` 's `x_pos` bit is
turn `v` into double `f` and `TestDouble(f)`
✅ dergoegge closed a pull request: "Revert "build: Fix regression in "ARMv8 CRC32 intrinsics" test""
(https://github.com/bitcoin/bitcoin/pull/29226)
(https://github.com/bitcoin/bitcoin/pull/29226)
💬 michaelfolkson commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911987679)
The incorrect use of terms like "exploit", "bug" and "spam" is ensuring this discussion is extremely subpar. The absolute best this pull request can accomplish is to slow down the confirmation of certain transactions and ensure they are submitted directly to mining pools rather than propagated over the P2P network. So even if it was an "exploit" (which it isn't) this pull request doesn't fix the "exploit".
If it truly was an "exploit" it would need to be comprehensively fixed. The way to comp
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911987679)
The incorrect use of terms like "exploit", "bug" and "spam" is ensuring this discussion is extremely subpar. The absolute best this pull request can accomplish is to slow down the confirmation of certain transactions and ensure they are submitted directly to mining pools rather than propagated over the P2P network. So even if it was an "exploit" (which it isn't) this pull request doesn't fix the "exploit".
If it truly was an "exploit" it would need to be comprehensively fixed. The way to comp
...
💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911994284)
> with negative effects on fee estimation.
What is the negative effect? Could you describe (and possibly quantify in order to put it into perspective), please?
Could you explain the mechanism or provide evidence that support the opinion, please? Why negative and not neutral? Isn't the fee estimation flawed and confused with the proposal, perhaps?
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911994284)
> with negative effects on fee estimation.
What is the negative effect? Could you describe (and possibly quantify in order to put it into perspective), please?
Could you explain the mechanism or provide evidence that support the opinion, please? Why negative and not neutral? Isn't the fee estimation flawed and confused with the proposal, perhaps?
🤔 shaavan reviewed a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-1845680627)
Concept ACK
<details>
<summary>
Notes:
</summary>
1. Change the type of nVersion for transactions from `int32_t` → `uint32_t`
2. Appropriately remove casts, from the codebase
3. Introduce test cases to properly address the change in behavior
4. Appropriately change the format form `<i` to `<I` indicating the change from signed little-endian to unsigned little-endian when serializing and deserializing tx.nVersion.
</details>
Suggestions:
1. In `void static RandomTransaction(CM
...
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-1845680627)
Concept ACK
<details>
<summary>
Notes:
</summary>
1. Change the type of nVersion for transactions from `int32_t` → `uint32_t`
2. Appropriately remove casts, from the codebase
3. Introduce test cases to properly address the change in behavior
4. Appropriately change the format form `<i` to `<I` indicating the change from signed little-endian to unsigned little-endian when serializing and deserializing tx.nVersion.
</details>
Suggestions:
1. In `void static RandomTransaction(CM
...
💬 shaavan commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467605265)
Question: If we are removing the "signedness" of the tx.nVersion, should we think about removing the test cases that made sense for the signed nVersion?
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467605265)
Question: If we are removing the "signedness" of the tx.nVersion, should we think about removing the test cases that made sense for the signed nVersion?
💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912006236)
> The absolute best this pull request can accomplish is to slow down the confirmation of certain transactions and ensure they are submitted directly to mining pools rather than propagated over the P2P network.
No, that's untrue. To the contrary, that pull requests doesn't ensure that transactions are submitted directly to mining pools rather than propagated over the P2P network.
In my view, the discussion here is about deficiency in Bitcoin Core implementation that does not support configu
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912006236)
> The absolute best this pull request can accomplish is to slow down the confirmation of certain transactions and ensure they are submitted directly to mining pools rather than propagated over the P2P network.
No, that's untrue. To the contrary, that pull requests doesn't ensure that transactions are submitted directly to mining pools rather than propagated over the P2P network.
In my view, the discussion here is about deficiency in Bitcoin Core implementation that does not support configu
...
💬 michaelfolkson commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912011343)
@GregTonoski:
> What is the negative effect? Could you describe (and possibly quantify in order to put it into perspective), please?
The point here is that if you prevent full nodes from receiving and verifying high fee rate, consensus (rule) valid transactions prior to them being mined then a full node is unaware of increasing numbers of high fee rate, consensus (rule) valid transactions until they are mined. Hence looking at their mempool and trying to work out what the current market fe
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912011343)
@GregTonoski:
> What is the negative effect? Could you describe (and possibly quantify in order to put it into perspective), please?
The point here is that if you prevent full nodes from receiving and verifying high fee rate, consensus (rule) valid transactions prior to them being mined then a full node is unaware of increasing numbers of high fee rate, consensus (rule) valid transactions until they are mined. Hence looking at their mempool and trying to work out what the current market fe
...
👍 shaavan approved a pull request: "doc: update `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1845698848)
ACK 437e8eb219c461d27c73a85e0c57d8e11b72a85d
Following the discussion in the PR so far, I agree that it is more logical and maintainable not to detail which function calls (or can call) a particular function since this information is easily accessible through other methods (e.g. by using an IDE)
That's why I think it's best to remove the comment.
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1845698848)
ACK 437e8eb219c461d27c73a85e0c57d8e11b72a85d
Following the discussion in the PR so far, I agree that it is more logical and maintainable not to detail which function calls (or can call) a particular function since this information is easily accessible through other methods (e.g. by using an IDE)
That's why I think it's best to remove the comment.
💬 knostr commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912023028)
> Nobody is defending the exploit. Rather, some of us recognize that this exploit is inherent in Bitcoin's design, whose script language provides plenty of room for embedding arbitrary data. What transactions miners put in blocks is driven purely by profit motives and the fact is that inscriptions are very profitable. Declaring them "bad" doesn't change that. As long as they follow consensus rules, they are "good" to the miners.
>
> The intentions behind this proposal are good. What I object
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912023028)
> Nobody is defending the exploit. Rather, some of us recognize that this exploit is inherent in Bitcoin's design, whose script language provides plenty of room for embedding arbitrary data. What transactions miners put in blocks is driven purely by profit motives and the fact is that inscriptions are very profitable. Declaring them "bad" doesn't change that. As long as they follow consensus rules, they are "good" to the miners.
>
> The intentions behind this proposal are good. What I object
...
💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912023953)
> @GregTonoski:
>
> > What is the negative effect? Could you describe (and possibly quantify in order to put it into perspective), please?
>
> (...) Hence looking at their mempool and trying to work out what the current market fee rate is distorted because many of the high fee rate, consensus (rule) valid transactions aren't in their mempool (...). The full node's mempool is being partially blinded to what high fee rate, consensus (rule) valid transactions are actually out there waiting to
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912023953)
> @GregTonoski:
>
> > What is the negative effect? Could you describe (and possibly quantify in order to put it into perspective), please?
>
> (...) Hence looking at their mempool and trying to work out what the current market fee rate is distorted because many of the high fee rate, consensus (rule) valid transactions aren't in their mempool (...). The full node's mempool is being partially blinded to what high fee rate, consensus (rule) valid transactions are actually out there waiting to
...
⚠️ fanquake opened an issue: "ci: failure in feature_index_prune.py (MSVC)"
(https://github.com/bitcoin/bitcoin/issues/29326)
https://github.com/bitcoin/bitcoin/actions/runs/7667501768/job/20897463395
```bash
============
Combined log for D:\a\_temp/test_runner_₿_🏃_20240126_113602/feature_index_prune_293:
============
test 2024-01-26T11:36:28.165000Z TestFramework (INFO): PRNG seed is: 8852427722333217080
test 2024-01-26T11:36:28.165000Z TestFramework (DEBUG): Setting up network thread
test 2024-01-26T11:36:28.212000Z TestFramework (INFO): Initializing test directory D:\a\_temp\test_runner_₿_🏃_202
...
(https://github.com/bitcoin/bitcoin/issues/29326)
https://github.com/bitcoin/bitcoin/actions/runs/7667501768/job/20897463395
```bash
============
Combined log for D:\a\_temp/test_runner_₿_🏃_20240126_113602/feature_index_prune_293:
============
test 2024-01-26T11:36:28.165000Z TestFramework (INFO): PRNG seed is: 8852427722333217080
test 2024-01-26T11:36:28.165000Z TestFramework (DEBUG): Setting up network thread
test 2024-01-26T11:36:28.212000Z TestFramework (INFO): Initializing test directory D:\a\_temp\test_runner_₿_🏃_202
...