💬 musaHaruna commented on pull request "mining: rename gbt_force and gbt_force_name":
(https://github.com/bitcoin/bitcoin/pull/32386#issuecomment-2851148905)
@Sjors Thanks. Updated to the latest hash
(https://github.com/bitcoin/bitcoin/pull/32386#issuecomment-2851148905)
@Sjors Thanks. Updated to the latest hash
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851165167)
> @BitcoinMechanic
>
> > no incentive
>
> Fee estimation and block propagation to name a few: https://groups.google.com/g/bitcoindev/c/d6ZO7gXGYbQ/m/3WVL60u6EQAJ
It does no harm to fee estimation or block propagation. Nodes can and do cache transactions they reject from their mempools making compact blocks just as quick to verify regardless of if some of their contents was filtered.
Fee estimation similarly does not require knowledge of "the" mempool and there can never be such a thi
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851165167)
> @BitcoinMechanic
>
> > no incentive
>
> Fee estimation and block propagation to name a few: https://groups.google.com/g/bitcoindev/c/d6ZO7gXGYbQ/m/3WVL60u6EQAJ
It does no harm to fee estimation or block propagation. Nodes can and do cache transactions they reject from their mempools making compact blocks just as quick to verify regardless of if some of their contents was filtered.
Fee estimation similarly does not require knowledge of "the" mempool and there can never be such a thi
...
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851168826)
Concept ACK. This adds a deprecation step to #32406, which seems fine from a technical point of view, and was requested by regular contributors as well.
It will re-invite the brigading when the actual code is removed, but it will be easier to point to earlier discussion.
Code looks reasonable at first glance, when compared to #32359, but will re-review it.
3ba7449f6c335026b752366c53f9c309f09e6c64 could be split between a commit that allows multiple outputs and one that switches the defa
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851168826)
Concept ACK. This adds a deprecation step to #32406, which seems fine from a technical point of view, and was requested by regular contributors as well.
It will re-invite the brigading when the actual code is removed, but it will be easier to point to earlier discussion.
Code looks reasonable at first glance, when compared to #32359, but will re-review it.
3ba7449f6c335026b752366c53f9c309f09e6c64 could be split between a commit that allows multiple outputs and one that switches the defa
...
🚀 hebasto merged a pull request: "build: replace header checks with `__has_include`"
(https://github.com/bitcoin/bitcoin/pull/32405)
(https://github.com/bitcoin/bitcoin/pull/32405)
💬 Retropex commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851173915)
Concept NACK.
For the same reasons mentioned in #32359.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851173915)
Concept NACK.
For the same reasons mentioned in #32359.
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2851174297)
@Sjors I just tested this by modifying the lines [here](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L97-L98) to be `1min` and `2min`, deleted my `blocks`, `chainstate`, and `indexes` directories and ran `./build/bin/bitcoind -maxconnections=0 -addnode=<local_node> -debug=coindb`. I can confirm it flushes between 1 and 2 minutes after startup and continuously after, due to the change in `chainstate` directory size and the `coindb` logs.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2851174297)
@Sjors I just tested this by modifying the lines [here](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L97-L98) to be `1min` and `2min`, deleted my `blocks`, `chainstate`, and `indexes` directories and ran `./build/bin/bitcoind -maxconnections=0 -addnode=<local_node> -debug=coindb`. I can confirm it flushes between 1 and 2 minutes after startup and continuously after, due to the change in `chainstate` directory size and the `coindb` logs.
💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2851176101)
What commit is that failure from?
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2851176101)
What commit is that failure from?
💬 nsvrn commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851209816)
Concept NACK
If fee estimation, block propagation etc has issues with mempool diversity specially for defense against spam/DoS then the position of not fixing that or making a future economic judgement based on theoretical assumptions is a fallacy. This broad position of Bitcoin Core can clearly increase spam and centralization regardless of economic incentives of miners which are all speculations and projections to modify how Bitcoin works instead of being realistic about the end goal of Bit
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851209816)
Concept NACK
If fee estimation, block propagation etc has issues with mempool diversity specially for defense against spam/DoS then the position of not fixing that or making a future economic judgement based on theoretical assumptions is a fallacy. This broad position of Bitcoin Core can clearly increase spam and centralization regardless of economic incentives of miners which are all speculations and projections to modify how Bitcoin works instead of being realistic about the end goal of Bit
...
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851218711)
FWIW I will not engage in meta discussion here. I will respond to any deficiencies in the code itself.
> https://github.com/bitcoin/bitcoin/commit/3ba7449f6c335026b752366c53f9c309f09e6c64 could be split between a commit that allows multiple outputs and one that switches the default.
Touching tests twice is kind of annoying, but I can do if it makes code history easier and others agree.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851218711)
FWIW I will not engage in meta discussion here. I will respond to any deficiencies in the code itself.
> https://github.com/bitcoin/bitcoin/commit/3ba7449f6c335026b752366c53f9c309f09e6c64 could be split between a commit that allows multiple outputs and one that switches the default.
Touching tests twice is kind of annoying, but I can do if it makes code history easier and others agree.
💬 pinheadmz commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851220227)
@nsvrn this PR specifically enables users to have a divergent mempool. I don't understand the use case for that, but the proposal here addresses feedback on #32359 and adds a feature specifically for users who do not want their mempool to anticipate miner behavior.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851220227)
@nsvrn this PR specifically enables users to have a divergent mempool. I don't understand the use case for that, but the proposal here addresses feedback on #32359 and adds a feature specifically for users who do not want their mempool to anticipate miner behavior.
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851220667)
@Sjors this is materially different from the other PR as it at least allows users the ability to configure. Not sure it's appropriate to invoke same reasoning in both cases?
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851220667)
@Sjors this is materially different from the other PR as it at least allows users the ability to configure. Not sure it's appropriate to invoke same reasoning in both cases?
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2851224058)
`61ba3fc494...9057ab8b80`:
> shouldn't `Truncate` and `Commit` also set `m_was_written = true`?
Good question! I think `Truncate()` - yes because it changes the contents of the file. Changed.
`Commit()` does not, so I left it alone. In other words - if a file is opened, none of the `write()`, `write_buffer()` or `Truncate()` is called, `Commit()` is called, and the file object is destroyed, could that be a problem? I think no because the contents of the file was not changed.
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2851224058)
`61ba3fc494...9057ab8b80`:
> shouldn't `Truncate` and `Commit` also set `m_was_written = true`?
Good question! I think `Truncate()` - yes because it changes the contents of the file. Changed.
`Commit()` does not, so I left it alone. In other words - if a file is opened, none of the `write()`, `write_buffer()` or `Truncate()` is called, `Commit()` is called, and the file object is destroyed, could that be a problem? I think no because the contents of the file was not changed.
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851232986)
> @nsvrn this PR specifically enables users to have a divergent mempool. I don't understand the use case for that
The use case for users maintaining control over their mempools is self evident.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851232986)
> @nsvrn this PR specifically enables users to have a divergent mempool. I don't understand the use case for that
The use case for users maintaining control over their mempools is self evident.
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851233257)
@BitcoinMechanic wrote:
> allows users the ability to configure
It briefly retains it, typically deprecation is followed by removal one release later.
So if you're opposed to direct removal, then everyone will understand that you're also opposed to deprecation, so there's nothing new to say.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851233257)
@BitcoinMechanic wrote:
> allows users the ability to configure
It briefly retains it, typically deprecation is followed by removal one release later.
So if you're opposed to direct removal, then everyone will understand that you're also opposed to deprecation, so there's nothing new to say.
🤔 polespinasa reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2815021971)
cACK
I like this approach, is far less problematic than https://github.com/bitcoin/bitcoin/pull/32359
Not going to add any more argument than the ones given there.
Left two small non-blocking comments
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2815021971)
cACK
I like this approach, is far less problematic than https://github.com/bitcoin/bitcoin/pull/32359
Not going to add any more argument than the ones given there.
Left two small non-blocking comments
💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073561758)
I think this should be a warning- something like:
```c++
warnings.push_back(_("Warning: Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated. They will be removed in future versions."));
```
See #31278
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073561758)
I think this should be a warning- something like:
```c++
warnings.push_back(_("Warning: Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated. They will be removed in future versions."));
```
See #31278
💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073586466)
A personal opinion to read the code (non blocking)
Maybe would be more clear to have it as a constant and override it if the option is set?
If not, maybe a comment would be good.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073586466)
A personal opinion to read the code (non blocking)
Maybe would be more clear to have it as a constant and override it if the option is set?
If not, maybe a comment would be good.
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073604279)
functional tests were failing on shutdown when `InitWarning` was being triggered on an earlier version of my code. Is there a way to make the tests ok with it being set for specific tests?
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073604279)
functional tests were failing on shutdown when `InitWarning` was being triggered on an earlier version of my code. Is there a way to make the tests ok with it being set for specific tests?
💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2851264287)
Some more "buggy" examples
- Find the file descriptor for the debug log, then log some bootleg messages (needs Linux due to `/proc/.../fd` usage)
```python
#!/usr/bin/env python3
# evil-signer1.py
import os, socket, sys
fd_root = f'/proc/{os.getpid()}/fd'
open_files = []
for fname in os.listdir(fd_root):
try:
fd = int(fname)
if fd < 3:
continue
link = os.readlink(os.path.join(fd_root, fname))
open_files.append((fd, link))
exc
...
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2851264287)
Some more "buggy" examples
- Find the file descriptor for the debug log, then log some bootleg messages (needs Linux due to `/proc/.../fd` usage)
```python
#!/usr/bin/env python3
# evil-signer1.py
import os, socket, sys
fd_root = f'/proc/{os.getpid()}/fd'
open_files = []
for fname in os.listdir(fd_root):
try:
fd = int(fname)
if fd < 3:
continue
link = os.readlink(os.path.join(fd_root, fname))
open_files.append((fd, link))
exc
...
🤔 ismaelsadeeq reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2814921270)
Concept ACK on relaxing the maximum capacity of the `OP_RETURN` size to the current transaction standardness limit.
1. This change will improve fee estimation in Bitcoin Core, as users will now be able to see the fee pressure from "data-carier" transactions that were previously broadcast out of band.
2. It will also improve block propagation time and make mining fairer, as data carrier transactions below standardness limit will now be relayed in nodes' mempools before being included in blo
...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2814921270)
Concept ACK on relaxing the maximum capacity of the `OP_RETURN` size to the current transaction standardness limit.
1. This change will improve fee estimation in Bitcoin Core, as users will now be able to see the fee pressure from "data-carier" transactions that were previously broadcast out of band.
2. It will also improve block propagation time and make mining fairer, as data carrier transactions below standardness limit will now be relayed in nodes' mempools before being included in blo
...
💬 ismaelsadeeq commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073589388)
From what I see from previous PR is that after deprecation; the feature will be removed in next release cycle, so maybe indicate that?
https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1838749101
If we will eventually deprecate this then let's indicate that.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073589388)
From what I see from previous PR is that after deprecation; the feature will be removed in next release cycle, so maybe indicate that?
https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1838749101
If we will eventually deprecate this then let's indicate that.