💬 Retropex commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876904596)
@glozow I see, the [status quo](https://x.com/achow101/status/1735310357386731533?s=20) only applies when it suits you...
It's 50/50 of (N)Ack, it's not what I call an uncontrovesial change...
You do well to mention this PR because strangely it was [not](https://x.com/achow101/status/1735310357386731533?s=20) merged because of this piece of text.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876904596)
@glozow I see, the [status quo](https://x.com/achow101/status/1735310357386731533?s=20) only applies when it suits you...
It's 50/50 of (N)Ack, it's not what I call an uncontrovesial change...
You do well to mention this PR because strangely it was [not](https://x.com/achow101/status/1735310357386731533?s=20) merged because of this piece of text.
💬 kristapsk commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876918909)
@Retropex She is right, if you want to revert doc, code should also be changed to reflect that.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876918909)
@Retropex She is right, if you want to revert doc, code should also be changed to reflect that.
💬 Retropex commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876927476)
The purpose of `-datacarriersize` has ALWAYS been, as its name suggests, to enable or deactivate the relay of data carrier transactions.
However, a bug to bypass this function was found on December 14, 2022.
Literally **no** developer in the world corrects flaws by changing the documentation.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876927476)
The purpose of `-datacarriersize` has ALWAYS been, as its name suggests, to enable or deactivate the relay of data carrier transactions.
However, a bug to bypass this function was found on December 14, 2022.
Literally **no** developer in the world corrects flaws by changing the documentation.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876929590)
>> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
> BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
ago, when Lightning didn't even exist, and RBF's intended purpose was just to
let people fee bump their own on-chain transactions. It was a much simpler
time, and even then it got a BIP.
A BIP was not required for RBF to be considered or merged. PR #6871 was merged on Nov 27, 2015.
After [discussion about adding docum
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876929590)
>> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
> BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
ago, when Lightning didn't even exist, and RBF's intended purpose was just to
let people fee bump their own on-chain transactions. It was a much simpler
time, and even then it got a BIP.
A BIP was not required for RBF to be considered or merged. PR #6871 was merged on Nov 27, 2015.
After [discussion about adding docum
...
📝 maflcko opened a pull request: "wallet: Fix use-after-free in WalletBatch::EraseRecords"
(https://github.com/bitcoin/bitcoin/pull/29176)
Creating a copy of the pointer to the underlying data of the stream is not enough to copy the data.
Currently this happens to work sometimes, because the stream may not immediately free unused memory. However, there is no guarantee by the stream interface to always behave this way. Also, if `vector::clear` is called on the underlying memory, any pointers to it are invalid.
Fix this, by creating a full copy of all bytes.
(https://github.com/bitcoin/bitcoin/pull/29176)
Creating a copy of the pointer to the underlying data of the stream is not enough to copy the data.
Currently this happens to work sometimes, because the stream may not immediately free unused memory. However, there is no guarantee by the stream interface to always behave this way. Also, if `vector::clear` is called on the underlying memory, any pointers to it are invalid.
Fix this, by creating a full copy of all bytes.
💬 maflcko commented on pull request "wallet: Fix use-after-free in WalletBatch::EraseRecords":
(https://github.com/bitcoin/bitcoin/pull/29176#issuecomment-1876939948)
I believe this bug existed since the code was written in commit 22401f17e026ead4bc3fe96967eec56a719a4f75.
If one wants to trigger the bug in any c++ standard library, forcing a clear to also shrink may work:
```diff
diff --git a/src/streams.h b/src/streams.h
index bc04a2babd..26320db602 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -10,6 +10,7 @@
#include <span.h>
#include <support/allocators/zeroafterfree.h>
#include <util/overflow.h>
+#include <util/vector.h>
#includ
...
(https://github.com/bitcoin/bitcoin/pull/29176#issuecomment-1876939948)
I believe this bug existed since the code was written in commit 22401f17e026ead4bc3fe96967eec56a719a4f75.
If one wants to trigger the bug in any c++ standard library, forcing a clear to also shrink may work:
```diff
diff --git a/src/streams.h b/src/streams.h
index bc04a2babd..26320db602 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -10,6 +10,7 @@
#include <span.h>
#include <support/allocators/zeroafterfree.h>
#include <util/overflow.h>
+#include <util/vector.h>
#includ
...
💬 fanquake commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876945410)
> It makes a minor difference of reporting the correct version number.
What do you mean by/where are we reporting the version number?
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876945410)
> It makes a minor difference of reporting the correct version number.
What do you mean by/where are we reporting the version number?
💬 crediblebytes commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876950847)
You never change software requirements to fit the functionality. You either revisit the requirements to "Build the right software" or make the fix to "Build the software right". There was no bug fix but a lazy man's attempt to hide the bug at best. Honestly this person would be reprimanded for such negligence. Put the textual description back to what it was and provide a fix. Ya'll puttin development to shame.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876950847)
You never change software requirements to fit the functionality. You either revisit the requirements to "Build the right software" or make the fix to "Build the software right". There was no bug fix but a lazy man's attempt to hide the bug at best. Honestly this person would be reprimanded for such negligence. Put the textual description back to what it was and provide a fix. Ya'll puttin development to shame.
📝 hebasto opened a pull request: "build: Fix check whether `-latomic` needed"
(https://github.com/bitcoin/bitcoin/pull/29177)
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
(https://github.com/bitcoin/bitcoin/pull/29177)
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
💬 fanquake commented on issue "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`":
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1876969998)
> fails with error: unknown argument: '-internal-isystem/usr/local/include'
Can this be investigated further. The `-Xclang` and `-internal-isystem` options still exist in Clang. Is the issue here just the concatenation? Maybe that worked previously, but no-longer does? What happens if you change `-Xclang -internal-isystem/usr/local/include` to `-Xclang -internal-isystem /usr/local/include`?
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1876969998)
> fails with error: unknown argument: '-internal-isystem/usr/local/include'
Can this be investigated further. The `-Xclang` and `-internal-isystem` options still exist in Clang. Is the issue here just the concatenation? Maybe that worked previously, but no-longer does? What happens if you change `-Xclang -internal-isystem/usr/local/include` to `-Xclang -internal-isystem /usr/local/include`?
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441658150)
Please see https://github.com/bitcoin/bitcoin/pull/29177.
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441658150)
Please see https://github.com/bitcoin/bitcoin/pull/29177.
💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876973577)
On Thu, Jan 04, 2024 at 01:06:19AM -0800, Bastien Teinturier wrote:
> > No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
>
> You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876973577)
On Thu, Jan 04, 2024 at 01:06:19AM -0800, Bastien Teinturier wrote:
> > No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
>
> You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since
...
💬 stickies-v commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876980521)
NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn't have incorrect documentation while that process plays out.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876980521)
NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn't have incorrect documentation while that process plays out.
💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876988120)
On Thu, Jan 04, 2024 at 03:19:50AM -0800, Gloria Zhao wrote:
> >> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
>
> > BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
> ago, when Lightning didn't even exist, and RBF's intended purpose was just to
> let people fee bump their own on-chain transactions. It was a much simpler
> time, and even then it got a BIP.
>
> A BIP was not required for RBF to be considered or merged. PR #
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876988120)
On Thu, Jan 04, 2024 at 03:19:50AM -0800, Gloria Zhao wrote:
> >> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
>
> > BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
> ago, when Lightning didn't even exist, and RBF's intended purpose was just to
> let people fee bump their own on-chain transactions. It was a much simpler
> time, and even then it got a BIP.
>
> A BIP was not required for RBF to be considered or merged. PR #
...
⚠️ fanquake opened an issue: "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc"
(https://github.com/bitcoin/bitcoin/issues/29178)
master @ 65c05db660b2ca1d0076b0d8573a6760b3228068.
Running `time FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh` on Rawhide aarch64.
```bash
Run coincontrol with args ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')]crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'u
...
(https://github.com/bitcoin/bitcoin/issues/29178)
master @ 65c05db660b2ca1d0076b0d8573a6760b3228068.
Running `time FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh` on Rawhide aarch64.
```bash
Run coincontrol with args ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')]crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'u
...
💬 cculianu commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877009300)
> > Concept ACK
> > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
>
> As long as miners continue to mine these transactions they wi
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877009300)
> > Concept ACK
> > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
>
> As long as miners continue to mine these transactions they wi
...
💬 maflcko commented on pull request "build: Fix check whether `-latomic` needed":
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033482)
Testing on a fresh install of Ubuntu 24.04 Noble: `export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 && cd bitcoin && git fetch origin --depth=1 f8ca1357c8205ceff732dcfb0d2bad79b40b611b && git checkout f8ca1357c8205ceff732dcfb0d2bad79b40b611b && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 make automake cmake curl clang-17 llvm-17 g++-multi
...
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033482)
Testing on a fresh install of Ubuntu 24.04 Noble: `export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 && cd bitcoin && git fetch origin --depth=1 f8ca1357c8205ceff732dcfb0d2bad79b40b611b && git checkout f8ca1357c8205ceff732dcfb0d2bad79b40b611b && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 make automake cmake curl clang-17 llvm-17 g++-multi
...
💬 maflcko commented on pull request "build: Fix check whether `-latomic` needed":
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033681)
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033681)
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
💬 michaelfolkson commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877045032)
Concept NACK.
1) The high fee rate, consensus valid transactions that Ocean mines is Ocean's business but trying to prevent high fee rate, consensus valid transactions from finding their way to **all** miners is a losing game. Even if this PR was merged the transactions could be submitted to miners out of band.
2) Transactions are just bytes with constraints imposed on them by the consensus rules. There is an endless list of ways small chunks of data (ie bytes) can be embedded in consensu
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877045032)
Concept NACK.
1) The high fee rate, consensus valid transactions that Ocean mines is Ocean's business but trying to prevent high fee rate, consensus valid transactions from finding their way to **all** miners is a losing game. Even if this PR was merged the transactions could be submitted to miners out of band.
2) Transactions are just bytes with constraints imposed on them by the consensus rules. There is an endless list of ways small chunks of data (ie bytes) can be embedded in consensu
...
💬 ajtowns commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877052212)
> You never change software requirements to fit the functionality.
The help text and documentation is not generally a statement of requirements, rather it documents what the software does, whether that's optimal/desirable or not. The closest thing we have to requirements docs are the BIPs, however the senior editor there [considers node policies](https://github.com/bitcoin/bips/pull/1524#issuecomment-1869734387) like this as being out of scope there as well.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877052212)
> You never change software requirements to fit the functionality.
The help text and documentation is not generally a statement of requirements, rather it documents what the software does, whether that's optimal/desirable or not. The closest thing we have to requirements docs are the BIPs, however the senior editor there [considers node policies](https://github.com/bitcoin/bips/pull/1524#issuecomment-1869734387) like this as being out of scope there as well.
💬 sipa commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877091223)
> What do you mean by/where are we reporting the version number?
The commit title, "Update secp256k1 subtree to upstream release 0.4.1", makes it sound like it's updating to the exact 0.4.1 tag. I agree it's fine to update to master, and that we shouldn't need a policy of only using tagged releases, but it'd be better if the commit title said "Update secp256k1 subtree to current master", or "to commit after 0.4.1".
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877091223)
> What do you mean by/where are we reporting the version number?
The commit title, "Update secp256k1 subtree to upstream release 0.4.1", makes it sound like it's updating to the exact 0.4.1 tag. I agree it's fine to update to master, and that we shouldn't need a policy of only using tagged releases, but it'd be better if the commit title said "Update secp256k1 subtree to current master", or "to commit after 0.4.1".